IWF-129: Enable executingStateIds SAs for states with waitUntil

[lwolczynski]

Description

Checklist

• Code compiles correctly
• Tests for the changes have been added
• All tests passing
• This PR change is backwards-compatible
• This PR CONTAINS a (planned) breaking change (it is not backwards compatible)

Related Issue

Closes #411

[longquanzheng]

This optimization is broken already for current default/ENABLED_FOR_ALL, and it won't work for ENABLED_FOR_STATES_WITH_WAIT_UNTIL. We need to fix it, and have to update ClearExecutingStateIdsSearchAttributeFinally as well.

To keep determinsm, we have to add a IsBeforeVersionOfExecutingStateIdMode in globalVersioner.
Then here:

if e.globalVersioner.IsAfterVersionOfOptimizedUpsertSearchAttribute() && 
e.globalVersioner.IsBeforeVersionOfExecutingStateIdMode() &&
len(executingStateIds) == 0 {
    // explain why we stopped using this optimization
    return nil
}

Similarly in ClearExecutingStateIdsSearchAttributeFinally:

if e.globalVersioner.IsAfterVersionOfOptimizedUpsertSearchAttribute() && 
e.globalVersioner.IsBeforeVersionOfExecutingStateIdMode() {
   if if config.GetDisableSystemSearchAttribute() {	
		return
   }
  if e.totalCurrentlyExecutingCount == 0{
      upsertSearchAttribute( []string{}) // clear the value.
   }
}

Btw, you can also create a method in globalVersioner IsAfterVersionOfOptimizedUpsertSearchAttributeAndBeforeVersionOfExecutingStateIdMode() to combinet this check. Maybe that will look better (maybe not, it looks hard to read actually)

[longquanzheng]

Btw, I realized that the correct way to optimize this UpsertSearchAttribute(which is a temporal action) is to check the next state like this: #454

We can also do it in next PR -- as long as we don't release the this PR to production, we don't have to create a new version (so that we don't keep too many versions)

[lwolczynski]

IsBeforeVersionOfExecutingStateIdMode is equal to !IsAfterVersionOfExecutingStateIdMode. Do we need an extra method for this check?

The latter method is somehow malformed in your snippet. I changed it to:

func (e *StateExecutionCounter) ClearExecutingStateIdsSearchAttributeFinally() {
	config := e.configer.Get()

	if e.globalVersioner.IsAfterVersionOfExecutingStateIdMode() {
		if config.GetExecutingStateIdMode() == "DISABLED" {
			return
		}
	} else {
		if config.GetDisableSystemSearchAttribute() {
			return
		}
	}

	if e.globalVersioner.IsAfterVersionOfOptimizedUpsertSearchAttribute() && !e.globalVersioner.IsAfterVersionOfExecutingStateIdMode() && e.totalCurrentlyExecutingCount == 0 {
		err := e.provider.UpsertSearchAttributes(e.ctx, map[string]interface{}{
			service.SearchAttributeExecutingStateIds: []string{},
		})
		if err != nil {
			e.provider.GetLogger(e.ctx).Error("error for upsert SearchAttributeExecutingStateIds", err)
		}
	}
}

We will need to refactor it further, since with how it is right now, SearchAttributeExecutingStateIds won't be cleared when e.globalVersioner.IsAfterVersionOfExecutingStateIdMode() == true.

And I'd rather work on #454 in a separate branch. I can rebase off of this one and merge #454 into here before merging to main if we want to release other changes.

[longquanzheng]

IsBeforeVersionOfExecutingStateIdMode is equal to !IsAfterVersionOfExecutingStateIdMode. Do we need an extra method for this check?

You are right, we don't have to. Sometimes I just prefer to not use negative statement/expression (reasons are like https://www.obsidiansecurity.com/blog/why-to-stop-writing-negative-code/ )

But no much difference and it's not always the case.

[longquanzheng]

And I'd rather work on #454 in a separate branch. I can rebase off of this one and merge #454 into here before merging to main if we want to release other changes.

+1
Sounds good. btw maybe not add replay tests for this PR . Otherwise you will have to do it again (maybe it's fine) if we want to reuse the same version (because that optimization is a non-determinstic change)

[longquanzheng]

This would work, but I usually prefer signal which is more controllable than time

[lwolczynski]

Simplified this test and removed the command entirely

[lwolczynski]

Add comments addressed. Moving on to #454