-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(stepfunctions): support Map ItemProcessor #27913
Conversation
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.
Some preliminary thoughts @lpizzinidev, but thanks for getting this started!
``` | ||
|
||
To define a distributed `Map` state use the configuration parameter. |
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.
unclear to me what the "configuration parameter" is
@@ -194,8 +207,16 @@ export class Map extends State implements INextable { | |||
protected validateState(): string[] { | |||
const errors: string[] = []; | |||
|
|||
if (this.iteration === undefined) { | |||
errors.push('Map state must have a non-empty iterator'); | |||
if (this.iteration === undefined && this.processor === undefined) { |
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 mean, this is fine, but why not if (!this.iteration && !this.processor)
@@ -162,13 +162,25 @@ export class Map extends State implements INextable { | |||
|
|||
/** | |||
* Define iterator state machine in Map | |||
* | |||
* @deprecated - use `itemProcessor` instead. | |||
*/ | |||
public iterator(iterator: IChainable): Map { |
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.
Docstring should mention mutex with itemProcessor
/** | ||
* Define item processor in Map | ||
*/ | ||
public itemProcessor(processor: IChainable, config: ProcessorConfig = {}): Map { |
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.
Docstring should mention mutex with iterator
return { | ||
Iterator: this.iteration.toGraphJson(), | ||
Iterator: this.iteration?.toGraphJson(), |
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.
don't need this ?
right? typescript should know whats up based on line 430
return { | ||
ItemProcessor: { | ||
...this.renderProcessorConfig(), | ||
...this.processor?.toGraphJson(), |
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.
Shouldn't TS know this.processor
exists without the ?
?
/** | ||
* Render ProcessorConfig in ASL JSON format | ||
*/ | ||
private renderProcessorConfig(): any { |
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.
private renderProcessorConfig(): any { | |
private renderProcessorConfig() { |
any
doesn't help much, i'd rather TS guses at the return type
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.
Makes sense, I specified the return type as this is the approach used for the other rendering functions.
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.
Fair :). If there's a reason for it I can't think of one, but it's whatever either way.
Pull request has been modified.
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Adds support for Map's [ItemSelector](https://docs.aws.amazon.com/step-functions/latest/dg/input-output-itemselector.html) field and deprecates [parameters](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-asl-use-map-state-inline.html#map-state-inline-deprecated-fields). With the release of Distributed Maps, AWS StepFunctions has released a few new fields, and deprecated two old ones for Map states. One of them was `Iterator` which was replaced with `ItemProcessor`, the other one is `Parameters` (replaced by `ItemSelector`). A similar PR was made to deprecate `Iterator` (#27913). While they are deprecated, these fields are still supported (PR reflects that) `ItemProcessor` and `ItemSelector` are both fields that are supported in `DISTRIBUTED` and `INLINE` mode, hence why they were added to the existing Map construct Closes #23265 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… executionType in the ProcessorConfig (#30301) ### Issue # (if applicable) Closes #30194 ### Reason for this change In #27913, the ItemProcessor was introduced for use with the Map Class. With the executionType in the ProcessorConfig, it was possible to specify the executionType for the Map. On the other hand, in #28821, the DistributedMap Class was introduced. The mapExecutionType of the DistributedMap class always overwrites the executionType of the ProcessorConfig. Therefore, when using the DistributedMap class, the implementation ignores the executionType of the ProcessorConfig. However, this behavior cannot be inferred from the documentation. ### Description of changes * Added to the docs that when using the DistributedMap Class, the executionType in the ProcessorConfig is ignored. * Also added a warning. ### Description of how you validated changes Add unit tests and integ tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… executionType in the ProcessorConfig (aws#30301) ### Issue # (if applicable) Closes aws#30194 ### Reason for this change In aws#27913, the ItemProcessor was introduced for use with the Map Class. With the executionType in the ProcessorConfig, it was possible to specify the executionType for the Map. On the other hand, in aws#28821, the DistributedMap Class was introduced. The mapExecutionType of the DistributedMap class always overwrites the executionType of the ProcessorConfig. Therefore, when using the DistributedMap class, the implementation ignores the executionType of the ProcessorConfig. However, this behavior cannot be inferred from the documentation. ### Description of changes * Added to the docs that when using the DistributedMap Class, the executionType in the ProcessorConfig is ignored. * Also added a warning. ### Description of how you validated changes Add unit tests and integ tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… executionType in the ProcessorConfig (aws#30301) ### Issue # (if applicable) Closes aws#30194 ### Reason for this change In aws#27913, the ItemProcessor was introduced for use with the Map Class. With the executionType in the ProcessorConfig, it was possible to specify the executionType for the Map. On the other hand, in aws#28821, the DistributedMap Class was introduced. The mapExecutionType of the DistributedMap class always overwrites the executionType of the ProcessorConfig. Therefore, when using the DistributedMap class, the implementation ignores the executionType of the ProcessorConfig. However, this behavior cannot be inferred from the documentation. ### Description of changes * Added to the docs that when using the DistributedMap Class, the executionType in the ProcessorConfig is ignored. * Also added a warning. ### Description of how you validated changes Add unit tests and integ tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds support for Map's
ItemProcessor
required field and deprecatesIterator
.Closes #27878.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license