-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(stepfunctions): DistributedMap ResultWriter correct query language selection
#35834
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
Conversation
730579a to
41b4aaf
Compare
kumvprat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Added a few inline comments
Can you also give me context into the feature flag being used in the test ?
Overall we should test with and without the feature flag being set and check for proper query language setting .
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map.ts
Outdated
Show resolved
Hide resolved
| // GIVEN | ||
| const stack = new cdk.Stack(); | ||
|
|
||
| stack.node.setContext(STEPFUNCTIONS_USE_DISTRIBUTED_MAP_RESULT_WRITER_V2, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we need to set explicitly ?
What about the case where this feature flag is false ? We need to add tests for that use case as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a defect of getResultWriter function
aws-cdk/packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map.ts
Lines 206 to 210 in de261c0
| private getResultWriter(): ResultWriterV2 | ResultWriter | undefined { | |
| return FeatureFlags.of(this).isEnabled(STEPFUNCTIONS_USE_DISTRIBUTED_MAP_RESULT_WRITER_V2) | |
| ? this.resultWriterV2 | |
| : this.resultWriter; | |
| } |
Currently, to use resultWriterV2 we need 2 steps:
- Set the flag to true
- Specify param
resultWriterV2: new stepfunctions.ResultWriterV2
I think only step 2 should be enough. But won't change this logic for now.
My intention when first submitting the PR was ignoring unit test for V1 as we're encouraging migration to V2. It is now added anw. Pls have a look.
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: phuhung273 <[email protected]>
Signed-off-by: phuhung273 <[email protected]>
41b4aaf to
2750938
Compare
Signed-off-by: phuhung273 <[email protected]>
...ramework-integ/test/aws-stepfunctions/test/integ.sm-jsonpath-with-distributed-map-jsonata.ts
Show resolved
Hide resolved
Signed-off-by: phuhung273 <[email protected]>
kumvprat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment in the test changes, apart from that it looks good.
|
Thanks so much for the help @kumvprat |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Close #35403
Reason for this change
Description of changes
Description of how you validated changes
Unit + Integ
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license