Fix workflow ID reuse when running on ScyllaDB

[PenguinToast]

What changed?

Fix an issue where the history service would get stuck in a loop when reusing workflow ID's on top of ScyllaDB.

Why?

Closes #2683.

How did you test it?
First reproduced the issue by running Temporal on top of Scylla locally; then verified that the change fixed the issue. Ran Cassandra persistence tests to ensure that this change didn't break anything else.

Potential risks
As noted in the linked issue:

Not sure if there are conditions when Cassandra might also return nullable rows.

If there are cases where we should be handling nil rows as real errors, this change would break that.

Is hotfix candidate?
Yes.

[CLAassistant]

All committers have signed the CLA.

[PenguinToast]

There's some tests failing; looking into them

[PenguinToast]

Fixed the tests; should be ready for review -- I'm not familiar with how to write idiomatic go; happy to make changes to this PR if needed.

cc @wxing1292 @yiminc

[brianlu-scale]

Oops, approved by accident, please ignore.

[alexshtin]

I am confused about this PR. newConflictRecord() essentially returns map[string]**int. Then you expect cassandra client to write *int into type field, and then you dereference it to int and store in the same type field. So what is the type of type field is expected by caller?

[PenguinToast]

I am confused about this PR. newConflictRecord() essentially returns map[string]**int. Then you expect cassandra client to write *int into type field, and then you dereference it to int and store in the same type field. So what is the type of type field is expected by caller?

We dereference and store to avoid changing all the downstream call sites that expect an int type -- happy to modify this PR to instead update those call sites to use an *int type instead, and update the relevant tests

[alexshtin]

I finally understood what you try to achieve here. I don't think that you need newConflictRecord func. It should work with default map[string]interface{} and here code should be:

Suggested change

	rowType, ok := conflictRecord["type"].(*int)
	if !ok || rowType == nil {
	return errors
	}
	// Dereference rowType for later use
	conflictRecord["type"] = *rowType
	// ScyllaDB will return rows with null values to match # of queries in a batch query (see #2683).
	// Field types will be pointer types (i.e. *int instead of int) in this case to support nil values.
	// Check only "type" field for simplicity.
	if rowType, ok := conflictRecord["type"].(*int); ok && rowType == nil {
	return nil
	}

and move this block before

var errors []error

line.

[PenguinToast]

If I don't use newConflictRecord, then ok will be false when I try to get conflictRecord["type"].(*int) -- I'm not familiar enough with Go to know exactly why it works like that, but I'm assuming this is related to how the Cassandra client library works.

[alexshtin]

Ok, there is definitely some sort of reflection magic inside Cassandra library. Can you try:

fmt.Printf("%T\n", conflictRecord["type"])

w/o newConflictRecord. It should print underlying type of "type" key. What is it?

[alexshtin]

It would be also great if you add test that fails in ScyllaDB w/o your fix and pass with it. Then I will be able to test it myself.

[v0id3r]

Ok, there is definitely some sort of reflection magic inside Cassandra library. Can you try:

fmt.Printf("%T\n", conflictRecord["type"])

w/o newConflictRecord. It should print underlying type of "type" key. What is it?

If I am not mistaken, this part does Unmarshalling.
https://github.com/gocql/gocql/blob/7a6cf00bbc98f4d7037e4a0fcca96fc946fd63d6/marshal.go#L205

We also used newConflictRecord way in our project to solve this issue, but @PenguinToast was faster :D
It is working on live env and seems to be no issues.

[alexshtin]

Can you test my suggestion with both DBs?

[PenguinToast]

Can you test my suggestion with both DBs?

@alexshtin Responded to your suggestion -- without pre-populating a pointer type in the conflictRecord map, ok will be false if we try to read a pointer type out of the conflictRecord

[alexshtin]

I played with it today. Yes, gocql uses reflection magic. I didn't like that "type" value got dereferenced and saved under the same key in the map. I removed dereferencing part and change all other code to use "type" as *int. Tested (run persistence unit tests) myself with both Cassandra and ScyllaDB.

Thanks for contribution!

[PenguinToast]

I played with it today. Yes, gocql uses reflection magic. I didn't like that "type" value got dereferenced and saved under the same key in the map. I removed dereferencing part and change all other code to use "type" as *int. Tested (run persistence unit tests) myself with both Cassandra and ScyllaDB.

Thanks for contribution!

Agree that it's much cleaner with your change -- thanks for reviewing & fixing!

[nicklaros]

I wonder why it is removed from release/1.17.2, we use Scylla and encounter this issue so we need this?

[alexshtin]

Our patch release strategy is to patch regression or new features that were introduced in corresponding minor version. This issue was always there and we decided to include it in the next minor version release (1.18).

[brianlu-scale]

@alexshtin Is there any estimate for when 1.18 will come out?