feat(stepfunctions): Distributed Map #23306

ayush987goyal · 2022-12-10T15:14:57Z

feat(stepfunctions): Distributed Map

Add native support for distributed map.

TODO: Write better documentation of the new features.

Refs:

closes #23216

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-12-10T15:15:07Z

aws-cdk-automation · 2022-12-11T05:58:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0caf146
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

patrickwerz · 2022-12-14T13:33:17Z

This would be helpful to have :)

Jacco · 2022-12-15T08:47:30Z

Would it be good to have the bucket in S3ReaderProps as an IBucket in stead of a string?

Also then it could be arranged the resource policy of the S3 bucket is amended properly?

mrgrain · 2022-12-16T09:17:13Z

packages/@aws-cdk/aws-stepfunctions/README.md

+
+Step Functions provides a high-concurrency mode for the Map state known as Distributed mode. In this mode, the Map state can accept input from large-scale Amazon S3 data sources. For example, your input can be a JSON or CSV file stored in an Amazon S3 bucket, or a JSON array passed from a previous step in the workflow. 
+
+A `Map` state with `mode` set to Distributed is known as a Distributed Map state. To create a Distributed Map state, set the `mode` property to `MapProcessorMode.DISTRIBUTED`. 


What's the benefit of this over let's say a new DistributedMap state?

mrgrain · 2022-12-16T09:20:15Z

packages/@aws-cdk/aws-stepfunctions/lib/state-machine.ts

@@ -500,6 +485,27 @@ export class StateMachine extends StateMachineBase {
      enabled: true,
    };
  }
+
+  private addDistributedMapStatePolicies(stateMachineName?: string) {


Making this optional and using the wildcard * later on seems like it's not required?
This is a private method and we only ever call it with the argument.

mrgrain · 2022-12-16T09:23:35Z

packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map/distributed-map-resource.ts

+/**
+ * Distributed Map State Resources
+ */
+export enum DistributedMapResource {


I don't think this provides a good user experience. What are these values for? What do they mean?
I could only find getObject in the docs you've linked.

Preferably we would refactor this to represent the semantic meaning instead of leaking the technical implementation. At least we should provide documentation explaining each of these.

mrgrain · 2022-12-16T09:27:23Z

packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map/item-reader.ts

+  /**
+   * The location of the header in the CSV file
+   *
+   * @default CSVHeaderLocation.FIRST_ROW
+   */
+  readonly headerLocation?: CSVHeaderLocation;
+
+  /**
+   * The headers of the CSV file
+   *
+   * @default - no headers
+   */
+  readonly headers?: string[];


Wondering about UX again. Maybe an enum like class would be better:

new S3CSVReader({ headers: CSVHeaders.useFirstRow(), });

new S3CSVReader({ headers: CSVHeaders.use(['one', 'two', 'three']), });

mrgrain · 2022-12-16T09:28:07Z

packages/@aws-cdk/aws-stepfunctions/lib/states/distributed-map/item-reader.ts

+  /**
+   * The S3 bucket
+   */
+  readonly bucket: string;


Any reasons not to use IBucket?

mrgrain · 2022-12-16T09:29:45Z

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts

+  private validateProps(props: MapProps) {
+    if (props.itemSelector && props.parameters) {
+      throw new Error('Only one of itemSelector or parameters can be provided');
+    }
+    if (props.mode === MapProcessorMode.INLINE && props.distributatedMapOptions) {
+      throw new Error('distributatedMapOptions can only be used with DISTRIBUTED mode');
+    }
+    if (props.distributatedMapOptions?.label && props.distributatedMapOptions?.label.length > 40) {
+      throw new Error('Label can only be 40 characters long');
+    }
+    if (props.distributatedMapOptions?.itemBatcher) {
+      const itemBatcher = props.distributatedMapOptions?.itemBatcher;
+      if (itemBatcher.maxItemsPerBatch && itemBatcher.maxItemsPerBatchPath) {
+        throw new Error('Only one of maxItemsPerBatch or maxItemsPerBatchPath can be provided');
+      }
+      if (itemBatcher.maxInputBytesPerBatch && itemBatcher.maxInputBytesPerBatchPath) {
+        throw new Error('Only one of maxInputBytesPerBatch or maxInputBytesPerBatchPath can be provided');
+      }
+    }
+  }


This long list of validations is an indication that we didn't get the UX right. There are so many ways a user can configure this wrong. We need to improve this by design.

mrgrain · 2022-12-16T09:30:31Z

packages/@aws-cdk/aws-stepfunctions/lib/states/map.ts

@@ -59,9 +166,17 @@ export interface MapProps {
   * The JSON that you want to override your default iteration input
   *
   * @default $
+   * @deprecated use `itemSelector` instead


Why? We need to also provide an explanation for deprecations. If not for the users, than for the people working on this code base.

This is deprecated by StepFunctions service: https://docs.aws.amazon.com/step-functions/latest/dg/input-output-itemselector.html

Would highly recommend going through the complete AWS documentation for the feature to understand how these things work together.

Exactly. This explanation should go into the docstring ;)

mrgrain

This looks great, but we need to iterate over the design. Or as a first start if you could share what your design thinking was.

ayush987goyal · 2022-12-16T15:27:10Z

@mrgrain Thanks for the comments and I do agree we could probably make it a bit simpler in some aspects. My overall thought around the design was:

To make it as smooth a transition from existing Map => DistributedMap so that it doesn't feel like a lot of overhead (StepFunction's docs also do not explicitly differentiate between these two maps, they only provide a opt-in flag for the same)
To have the API similar to the documentation so that people reading the docs should understand how those bits related to the API in CDK. This has been one of the principals of writing these L2 constructs but of-course these are not set to stone.

Feel free to provide any other comments/thoughts and we can take it from there.

rishisubramanian · 2022-12-29T21:55:33Z

Does this support the tolerated failure threshold parameters? I don't see them in the diff.

https://docs.aws.amazon.com/step-functions/latest/dg/maprun-fail-threshold.html

aws-cdk-automation · 2023-01-07T00:08:42Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

ayush987goyal · 2023-01-08T07:42:16Z

@mrgrain I have marked as re-requested for review since I want to have someone form CDK team review it as well once so that I do any major refactoring only once and I do not want this PR to be closed by the bot.

mrgrain · 2023-01-09T10:16:35Z

@mrgrain I have marked as re-requested for review since I want to have someone form CDK team review it as well once so that I do any major refactoring only once and I do not want this PR to be closed by the bot.

Hi @ayush987goyal I'm not sure I understand what you mean by this, I am from the CDK team and I kindly ask you to address my specific comments (either by code change or discussion) to move this forward.

If you are concerned (which is understandable) about multiple rounds of refactoring, maybe our RFC process would be more suitable for you. I'd say this feature is just on the edge of needing a RFC or just being a PR.

Finally, you could also publish a custom construct independently of the core package. This way you can gauge interest and confirm the API before graduating it to core.

aws-cdk-automation · 2023-01-15T00:09:06Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

jacob-kleidman · 2023-02-13T17:29:33Z

Hi @ayush987goyal; has this work been genuinely abandoned?

I really hope not 😟. I think this Distributed Map is critical to be inside the main CDK repo. I've been waiting on this for a while now

MorelSerge · 2023-02-14T03:35:30Z

Also very much looking forward to using this out of the box!

patrickwerz · 2023-02-22T17:02:51Z

ayush987goyal, could you create a cdk-constructs package of this? I would be super thankful to use your patch from there.

beck3905 · 2023-02-27T17:18:15Z

I created a new PR for this feature linked above. I tried to take some of the feedback from this PR into account. Thanks @ayush987goyal for some inspiration and concepts.

brian-moore · 2023-03-29T23:00:09Z

For anyone else looking for the PR @beck3905 mentioned above

#24331

feat(stepfunctions): Distributed Map

3df91d6

aws-cdk-automation requested a review from a team December 10, 2022 15:15

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Dec 10, 2022

ayush987goyal added 2 commits December 11, 2022 01:26

Update policy and add token check

7371b50

Update default policy for map

0caf146

mrgrain reviewed Dec 16, 2022

View reviewed changes

mrgrain requested changes Dec 16, 2022

View reviewed changes

ayush987goyal requested a review from mrgrain January 8, 2023 07:42

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jan 15, 2023

aws-cdk-automation closed this Jan 15, 2023

beck3905 mentioned this pull request Feb 24, 2023

feat(stepfunctions): distributed map state #24331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions): Distributed Map #23306

feat(stepfunctions): Distributed Map #23306

ayush987goyal commented Dec 10, 2022

gitpod-io bot commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

patrickwerz commented Dec 14, 2022

Jacco commented Dec 15, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain Dec 16, 2022

ayush987goyal Dec 16, 2022

mrgrain Dec 16, 2022

mrgrain left a comment

ayush987goyal commented Dec 16, 2022

rishisubramanian commented Dec 29, 2022

aws-cdk-automation commented Jan 7, 2023

ayush987goyal commented Jan 8, 2023

mrgrain commented Jan 9, 2023

aws-cdk-automation commented Jan 15, 2023

jacob-kleidman commented Feb 13, 2023

MorelSerge commented Feb 14, 2023

patrickwerz commented Feb 22, 2023

beck3905 commented Feb 27, 2023

brian-moore commented Mar 29, 2023


		Step Functions provides a high-concurrency mode for the Map state known as Distributed mode. In this mode, the Map state can accept input from large-scale Amazon S3 data sources. For example, your input can be a JSON or CSV file stored in an Amazon S3 bucket, or a JSON array passed from a previous step in the workflow.

		A `Map` state with `mode` set to Distributed is known as a Distributed Map state. To create a Distributed Map state, set the `mode` property to `MapProcessorMode.DISTRIBUTED`.

feat(stepfunctions): Distributed Map #23306

feat(stepfunctions): Distributed Map #23306

Conversation

ayush987goyal commented Dec 10, 2022

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

AWS CodeBuild CI Report

patrickwerz commented Dec 14, 2022

Jacco commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

ayush987goyal commented Dec 16, 2022

rishisubramanian commented Dec 29, 2022

aws-cdk-automation commented Jan 7, 2023

ayush987goyal commented Jan 8, 2023

mrgrain commented Jan 9, 2023

aws-cdk-automation commented Jan 15, 2023

jacob-kleidman commented Feb 13, 2023

MorelSerge commented Feb 14, 2023

patrickwerz commented Feb 22, 2023

beck3905 commented Feb 27, 2023

brian-moore commented Mar 29, 2023