refactor(stepfunctions-tasks): make integrationPattern an enum #3115

wqzoww · 2019-06-28T00:12:57Z

Step Functions allows users to call different integrated services in different, called service integration patterns. Instead of using booleans, this commit introduces a new property, integrationPattern, for all stepfunctions tasks which indicates the integration pattern (fire and forget, synchronous call, or wait for callback).

BREAKING CHANGE: If you used to pass waitForTaskToken: true you now need to pass integrationPattern: sfn.ServiceIntegrationPattern.WAIT_FOR_CALLBACK. If you used to pass sync: true, now pass integrationPattern: sfn.ServiceIntegrationPattern.SYNC. In addition, this commit enables users to define callback task with ECS.

Closes #3114

Please read the contribution guidelines and follow the pull-request checklist.

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

wqzoww · 2019-06-28T17:56:14Z

Although I managed to run and test the module in my local, the automatic checks failed.

The Travis CI build failure was caused by the fact that this code change does not guarantee backwards compatibility. Some boolean parameters have been removed and a new enum parameter has been added in place.

Please let me know if I can do something to skip this. I look forward to your feedback.

packages/@aws-cdk/aws-stepfunctions-tasks/lib/publish-to-topic.ts

packages/@aws-cdk/aws-stepfunctions-tasks/lib/run-ecs-task-base.ts

rix0rrr · 2019-08-05T08:56:21Z

packages/@aws-cdk/aws-stepfunctions/lib/step-functions-task.ts

+  /**
+   * Call a service and have Step Functions wait for a job to complete.
+   */
+  RUN_A_JOB = '.sync',


I don't like very much that these enum values are used for computation. Please just use symbolic string names and add a lookup table to transform them into the appropriate suffixes.

Also, are you sure RUN_A_JOB conveys the synchronicity of the action clearly enough?

I defined a map of <ServiceIntegrationPattern, string> to contain the suffix of each integration pattern.

Also, renamed RUN_A_JOB as SYNC.

rix0rrr · 2019-08-05T08:58:17Z

packages/@aws-cdk/aws-stepfunctions/lib/step-functions-task.ts

+  /**
+   * Call a service and let Step Functions progress to the next state immediately after it gets an HTTP response.
+   */
+  REQUEST_RESPONSE = '',


I know this is your terminology and so we should keep it, but this identifier is not at all clear to me.

The expected behavior is more akin to FIRE_AND_FORGET, no? Or API_CALL_ONLY? Can we do something about the docstring, maybe name it something like "[...] to the next state immediately after the API call completes" ?

I renamed REQUEST_RESPONSE as FIRE_AND_FORGET and RUN_A_JOB as SYNC.

For clarity, I'm not mandating: I'm only trying to engage you in discussion here. Since I believe "request response" is the terminology used in your official documentation, I'm not sure it's worth changing the term since it may confuse users.

If you as a service owner are okay with deviating from the terminology as a conscious choice, I would welcome the change. If the terminology should stay the same, I completely understand, but then we should make the behavior very clear in the doc string.

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-train-task.ts

packages/@aws-cdk/aws-stepfunctions-tasks/lib/sagemaker-transform-task.ts

packages/@aws-cdk/aws-stepfunctions-tasks/lib/send-to-queue.ts

rix0rrr · 2019-08-05T09:04:05Z

packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs-tasks.test.ts

@@ -57,6 +57,7 @@ test('Running a Fargate Task', () => {

  // WHEN
  const runTask = new sfn.Task(stack, 'RunFargate', { task: new tasks.RunEcsFargateTask({
+    serviceIntegrationPattern: sfn.ServiceIntegrationPattern.RUN_A_JOB,


Does synchronous or asynchronous make more sense as a default? What are users most likely to want?

rix0rrr

Ready to ship this given:

The lookup map is moved to the -tasks module.
You indicate you are comfortable with the FIRE_AND_FORGET rename.

rix0rrr · 2019-08-07T08:10:11Z

packages/@aws-cdk/aws-stepfunctions/lib/step-functions-task.ts

+  WAIT_FOR_TASK_TOKEN = 'WAIT_FOR_TASK_TOKEN'
+}
+
+export const resourceArnSuffix = new Map<ServiceIntegrationPattern, string>();


I'd rather this map lives in the aws-stepfunctions-tasks library (NOT exported from index.ts).

Thank you for the quick reply. I will move this map to aws-stepfunctions-tasks.

rix0rrr · 2019-08-07T08:22:30Z

In the interest of expediency and also integrating the changes on the other PR, if I don't get to it anymore, anyone on Seattle time is free to merge this once the requested changes have been addressed.

…erns for tasks Step Functions allows users to call different integrated services in different ways. They are also called service integration patterns, including Request Response, Run a Job and Wait for Callback. Users must choose exactly one of them and specify it in the "Resource" field. This commit introduces a new member variable, serviceIntegrationPattern, in the interface of properties within each existing integrated service. This helps to avoid using multiple boolean variables in the service such as ECS, which supports different service integration patterns. It is also beneficial for code maintenances: if Step Functions adds new integrated services or updates the existing integration patterns in the future, keeping pace with these changes will be simply updating this variable of enum type. BREAKING CHANGE: To define a callback task, users should specify "serviceIntegrationPattern: sfn.ServiceIntegrationPattern.WAIT_FOR_TASK_TOKEN" instead of "waitForTaskToken: true". For a sync task, users should use "serviceIntegrationPattern: sfn.ServiceIntegrationPattern.SYNC" in the place of "synchronous: true". In addition, this commit enables users to define callback task with ECS. **@aws-cdk/aws-stepfunctions-tasks** Closes aws#3114

eladb · 2019-07-23T14:00:06Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/publish-to-topic.ts

   */
-  readonly waitForTaskToken?: boolean;
+  readonly serviceIntegrationPattern?: sfn.ServiceIntegrationPattern;


Rename to integrationPattern

wqzoww requested a review from a team as a code owner June 28, 2019 00:12

ghost requested a review from rix0rrr June 28, 2019 00:13

wqzoww force-pushed the master branch from b27b964 to c401960 Compare June 28, 2019 03:11

NGL321 assigned rix0rrr Aug 1, 2019

rix0rrr reviewed Aug 5, 2019

View reviewed changes

wqzoww force-pushed the master branch from c401960 to 245b74c Compare August 7, 2019 05:59

rix0rrr mentioned this pull request Aug 7, 2019

feat(stepfunctions): support step functions state machine execution from a task state #3522

Merged

rix0rrr reviewed Aug 7, 2019

View reviewed changes

wqzoww force-pushed the master branch from 245b74c to 83c9737 Compare August 7, 2019 18:13

eladb reviewed Aug 8, 2019

View reviewed changes

serviceIntegrationPattern -> integrationPattern

5eeee6f

rix0rrr changed the title ~~refactor(aws-stepfunctions-tasks): implement service integration patt…~~ refactor(stepfunctions-tasks): integrationPattern is now an enum Aug 8, 2019

rix0rrr changed the title ~~refactor(stepfunctions-tasks): integrationPattern is now an enum~~ refactor(stepfunctions-tasks): make integrationPattern an enum Aug 8, 2019

rix0rrr approved these changes Aug 8, 2019

View reviewed changes

mergify bot added 2 commits August 8, 2019 10:09

Merge branch 'master' into master

6686f24

Merge branch 'master' into master

af4c1f2

mergify bot merged commit fa48e89 into aws:master Aug 8, 2019

rix0rrr added the medium-review label Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(stepfunctions-tasks): make integrationPattern an enum #3115

refactor(stepfunctions-tasks): make integrationPattern an enum #3115

wqzoww commented Jun 28, 2019 •

edited by rix0rrr

wqzoww commented Jun 28, 2019

rix0rrr Aug 5, 2019

rix0rrr Aug 5, 2019

wqzoww Aug 7, 2019

rix0rrr Aug 5, 2019

wqzoww Aug 6, 2019

rix0rrr Aug 7, 2019

rix0rrr Aug 5, 2019

rix0rrr left a comment

rix0rrr Aug 7, 2019

wqzoww Aug 7, 2019

rix0rrr commented Aug 7, 2019

eladb Jul 23, 2019

refactor(stepfunctions-tasks): make integrationPattern an enum #3115

refactor(stepfunctions-tasks): make integrationPattern an enum #3115

Conversation

wqzoww commented Jun 28, 2019 • edited by rix0rrr

wqzoww commented Jun 28, 2019

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

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Aug 7, 2019

Choose a reason for hiding this comment

wqzoww commented Jun 28, 2019 •

edited by rix0rrr