feat(stepfunctions-tasks): add timeout parameter for EmrCreateClust…

…er (aws#28532) This PR adds a new parameter `timeout` as Duration type instead of `timeoutDurationMinutes` because the `timeoutDurationMinutes` is a number type. Originally, `timeoutDurationMinutes` was a **required** parameter, but we have made it **optional** and also made the new parameter **optional** to avoid breaking change. Instead, added a validation to ensure that the value is specified. We discussed this in the following thread: aws#28529 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
paulhcsun · Jan 5, 2024 · f63d0a9 · f63d0a9
1 parent 17ee5a3
commit f63d0a9
Show file tree

Hide file tree

Showing 5 changed files with 149 additions and 14 deletions.
diff --git a/...est/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-spot-instance-fleet.ts b/...est/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-spot-instance-fleet.ts
@@ -1,5 +1,5 @@
 import * as sfn from 'aws-cdk-lib/aws-stepfunctions';
-import { App, Stack } from 'aws-cdk-lib';
+import { App, Duration, Stack } from 'aws-cdk-lib';
 import { IntegTest } from '@aws-cdk/integ-tests-alpha';
 import { EmrCreateCluster } from 'aws-cdk-lib/aws-stepfunctions-tasks';
 
@@ -20,7 +20,7 @@ const step = new EmrCreateCluster(stack, 'EmrCreateCluster', {
         spotSpecification: {
           allocationStrategy: EmrCreateCluster.SpotAllocationStrategy.CAPACITY_OPTIMIZED,
           timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
-          timeoutDurationMinutes: 60,
+          timeout: Duration.minutes(60),
         },
       },
       name: 'Master',

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md b/packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md
@@ -634,7 +634,7 @@ new tasks.EmrCreateCluster(this, 'SpotSpecification', {
         spotSpecification: {
           allocationStrategy: tasks.EmrCreateCluster.SpotAllocationStrategy.CAPACITY_OPTIMIZED,
           timeoutAction: tasks.EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
-          timeoutDurationMinutes: 60,
+          timeout: Duration.minutes(5),
         },
       },
     }],

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
@@ -732,9 +732,26 @@ export namespace EmrCreateCluster {
     /**
      * The spot provisioning timeout period in minutes.
      *
-     * The value must be between 5 and 1440.
+     * The value must be between 5 and 1440 minutes.
+     *
+     * You must specify one of `timeout` and `timeoutDurationMinutes`.
+     *
+     * @default - The value in `timeout` is used
+     *
+     * @deprecated - Use `timeout`.
+     */
+    readonly timeoutDurationMinutes?: number;
+
+    /**
+     * The spot provisioning timeout period in minutes.
+     *
+     * The value must be between 5 and 1440 minutes.
+     *
+     * You must specify one of `timeout` and `timeoutDurationMinutes`.
+     *
+     * @default - The value in `timeoutDurationMinutes` is used
      */
-    readonly timeoutDurationMinutes: number;
+    readonly timeout?: cdk.Duration;
   }
 
   /**

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts
@@ -150,14 +150,20 @@ function SpotProvisioningSpecificationPropertyToJson(property?: EmrCreateCluster
   if (!property) {
     return undefined;
   }
-  if (!cdk.Token.isUnresolved(property.timeoutDurationMinutes) && (property.timeoutDurationMinutes < 5 || property.timeoutDurationMinutes > 1440)) {
-    throw new Error(`timeoutDurationMinutes must be between 5 and 1440, got ${property.timeoutDurationMinutes}`);
+
+  if ((property.timeout && property.timeoutDurationMinutes) || (!property.timeout && !property.timeoutDurationMinutes)) {
+    throw new Error('one of timeout and timeoutDurationMinutes must be specified');
+  }
+  const timeout = property.timeout?.toMinutes() ?? property.timeoutDurationMinutes;
+  if (timeout !== undefined && !cdk.Token.isUnresolved(timeout) && (timeout < 5 || timeout > 1440)) {
+    throw new Error(`timeout must be between 5 and 1440 minutes, got ${timeout} minutes.`);
   }
+
   return {
     AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy),
     BlockDurationMinutes: cdk.numberToCloudFormation(property.blockDurationMinutes),
     TimeoutAction: cdk.stringToCloudFormation(property.timeoutAction?.valueOf()),
-    TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeoutDurationMinutes),
+    TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeout?.toMinutes() || property.timeoutDurationMinutes),
   };
 }
 

diff --git a/packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts b/packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
@@ -921,7 +921,7 @@ test.each([
             allocationStrategy: strategy,
             blockDurationMinutes: 1,
             timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
-            timeoutDurationMinutes: 5,
+            timeout: cdk.Duration.minutes(5),
           },
         },
         name: 'Main',
@@ -1033,7 +1033,7 @@ test('Create Cluster with InstanceFleet for Spot instances', () => {
           spotSpecification: {
             blockDurationMinutes: 1,
             timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
-            timeoutDurationMinutes: 5,
+            timeout: cdk.Duration.minutes(5),
           },
         },
         name: 'Main',
@@ -1219,7 +1219,63 @@ test('Create Cluster with InstanceFleet for On-Demand instances', () => {
   });
 });
 
-test('Throws if timeoutDurationMinutes for Spot instances is less than 5', () => {
+test('Throws if timeout for Spot instances is less than 5 minutes', () => {
+  // GIVEN
+  const task = new EmrCreateCluster(stack, 'Task', {
+    instances: {
+      instanceFleets: [{
+        instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
+        launchSpecifications: {
+          spotSpecification: {
+            timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
+            timeout: cdk.Duration.minutes(4),
+          },
+        },
+        name: 'Main',
+        targetSpotCapacity: 1,
+      }],
+    },
+    clusterRole,
+    name: 'Cluster',
+    serviceRole,
+    integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
+  });
+
+  // THEN
+  expect(() => {
+    task.toStateJson();
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 4/);
+});
+
+test('Throws if timeout for Spot instances is greater than 1440 minutes', () => {
+  // GIVEN
+  const task = new EmrCreateCluster(stack, 'Task', {
+    instances: {
+      instanceFleets: [{
+        instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
+        launchSpecifications: {
+          spotSpecification: {
+            timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
+            timeout: cdk.Duration.minutes(1441),
+          },
+        },
+        name: 'Main',
+        targetSpotCapacity: 1,
+      }],
+    },
+    clusterRole,
+    name: 'Cluster',
+    serviceRole,
+    integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
+  });
+
+  // THEN
+  expect(() => {
+    task.toStateJson();
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 1441 minutes./);
+});
+
+test('Throws if timeoutDurationMinutes for Spot instances is less than 5 minutes', () => {
   // GIVEN
   const task = new EmrCreateCluster(stack, 'Task', {
     instances: {
@@ -1244,10 +1300,10 @@ test('Throws if timeoutDurationMinutes for Spot instances is less than 5', () =>
   // THEN
   expect(() => {
     task.toStateJson();
-  }).toThrow(/timeoutDurationMinutes must be between 5 and 1440, got 4/);
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 4 minutes./);
 });
 
-test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440', () => {
+test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440 minutes', () => {
   // GIVEN
   const task = new EmrCreateCluster(stack, 'Task', {
     instances: {
@@ -1272,7 +1328,63 @@ test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440',
   // THEN
   expect(() => {
     task.toStateJson();
-  }).toThrow(/timeoutDurationMinutes must be between 5 and 1440, got 1441/);
+  }).toThrow(/timeout must be between 5 and 1440 minutes, got 1441 minutes./);
+});
+
+test('Throws if neither timeout nor timeoutDurationMinutes is specified', () => {
+  // GIVEN
+  const task = new EmrCreateCluster(stack, 'Task', {
+    instances: {
+      instanceFleets: [{
+        instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
+        launchSpecifications: {
+          spotSpecification: {
+            timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
+          },
+        },
+        name: 'Main',
+        targetSpotCapacity: 1,
+      }],
+    },
+    clusterRole,
+    name: 'Cluster',
+    serviceRole,
+    integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
+  });
+
+  // THEN
+  expect(() => {
+    task.toStateJson();
+  }).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
+});
+
+test('Throws if both timeout and timeoutDurationMinutes are specified', () => {
+  // WHEN
+  const task = new EmrCreateCluster(stack, 'Task', {
+    instances: {
+      instanceFleets: [{
+        instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
+        launchSpecifications: {
+          spotSpecification: {
+            timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
+            timeout: cdk.Duration.minutes(5),
+            timeoutDurationMinutes: 10,
+          },
+        },
+        name: 'Main',
+        targetSpotCapacity: 1,
+      }],
+    },
+    clusterRole,
+    name: 'Cluster',
+    serviceRole,
+    integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
+  });
+
+  // THEN
+  expect(() => {
+    task.toStateJson();
+  }).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
 });
 
 test('Throws if both bidPrice and bidPriceAsPercentageOfOnDemandPrice are specified', () => {