Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IWF-372: Fix GetSearchAttributes returning nil when no IwfExecutionStateIds are set #505

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

lwolczynski
Copy link
Contributor

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 #issue_number

@lwolczynski lwolczynski force-pushed the jira/lwolczynski/IWF-372 branch from 9c505c6 to 656aac8 Compare December 3, 2024 22:19
@@ -45,7 +45,8 @@ func MapToInternalSearchAttributes(attributes []iwfidl.SearchAttribute) (map[str

func MapCadenceToIwfSearchAttributes(searchAttributes *shared.SearchAttributes, requestedSearchAttributes []iwfidl.SearchAttributeKeyAndType) (map[string]iwfidl.SearchAttribute, error) {
if searchAttributes == nil || len(requestedSearchAttributes) == 0 {
return nil, nil
// return empty map rather than nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unable to convert value <nil> to type KEYWORD_ARRAY for key IwfExecutingStateIds

The error was when we converting a nil to KeywordArray, so I believe it's at line 66...

You should be able to reproduce this, if you modify wait_until_search_attributes_optimization_test.go to make it more complex -- for the default mode (ENABLED_FOR_STATES_WITH_WAIT_UNTIL_ONLY) , after the S7-1, start another S8 with waitUntil and it will try to refresh IwfExecutingStateIds with null value...

Screenshot 2024-12-03 at 2 41 02 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, whenever it tried to refresh IwfExecutingStateIds that is nil. At the beginning this SA doesn't even exist so there wasn't error.

Then it was set to some values(array), and then cleared up as a nil. Then setting again will see the errors.

Copy link
Contributor

@longquanzheng longquanzheng Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another easy way to produce this is using Java SDK:
S1->S2->S1 where S1 has waitUntil but S2 doesn't have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed + made an existing test to verify it works as expected

@longquanzheng
Copy link
Contributor

I also think we should change refreshIwfExecutingStateIdSearchAttribute to return the error instead of logging it.

Potentially, swallowing error and proceeding will cause wrong behavior -- we were super lucky last time that swallowing error was correct in that case 😂

By returning error, the workflow will be blocked and we have to fix the bug.

@lwolczynski lwolczynski force-pushed the jira/lwolczynski/IWF-372 branch from bb73c66 to b3cd703 Compare December 4, 2024 21:42
@lwolczynski lwolczynski force-pushed the jira/lwolczynski/IWF-372 branch from b3cd703 to dd70ab2 Compare December 4, 2024 21:44
@lwolczynski lwolczynski enabled auto-merge (squash) December 6, 2024 16:51
@lwolczynski lwolczynski merged commit a33475d into main Dec 6, 2024
10 checks passed
@lwolczynski lwolczynski deleted the jira/lwolczynski/IWF-372 branch December 6, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants