Skip to content

Commit ca91626

Browse files
go-to-kgo-to-k
authored
feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster (#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: #28529 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 5dd8072 commit ca91626

File tree

5 files changed

+149
-14
lines changed

5 files changed

+149
-14
lines changed

‎packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-spot-instance-fleet.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as sfn from 'aws-cdk-lib/aws-stepfunctions';
2-
import { App, Stack } from 'aws-cdk-lib';
2+
import { App, Duration, Stack } from 'aws-cdk-lib';
33
import { IntegTest } from '@aws-cdk/integ-tests-alpha';
44
import { EmrCreateCluster } from 'aws-cdk-lib/aws-stepfunctions-tasks';
55

@@ -20,7 +20,7 @@ const step = new EmrCreateCluster(stack, 'EmrCreateCluster', {
2020
spotSpecification: {
2121
allocationStrategy: EmrCreateCluster.SpotAllocationStrategy.CAPACITY_OPTIMIZED,
2222
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
23-
timeoutDurationMinutes: 60,
23+
timeout: Duration.minutes(60),
2424
},
2525
},
2626
name: 'Master',

‎packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,7 @@ new tasks.EmrCreateCluster(this, 'SpotSpecification', {
634634
spotSpecification: {
635635
allocationStrategy: tasks.EmrCreateCluster.SpotAllocationStrategy.CAPACITY_OPTIMIZED,
636636
timeoutAction: tasks.EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
637-
timeoutDurationMinutes: 60,
637+
timeout: Duration.minutes(5),
638638
},
639639
},
640640
}],

‎packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts

+19-2
Original file line numberDiff line numberDiff line change
@@ -732,9 +732,26 @@ export namespace EmrCreateCluster {
732732
/**
733733
* The spot provisioning timeout period in minutes.
734734
*
735-
* The value must be between 5 and 1440.
735+
* The value must be between 5 and 1440 minutes.
736+
*
737+
* You must specify one of `timeout` and `timeoutDurationMinutes`.
738+
*
739+
* @default - The value in `timeout` is used
740+
*
741+
* @deprecated - Use `timeout`.
742+
*/
743+
readonly timeoutDurationMinutes?: number;
744+
745+
/**
746+
* The spot provisioning timeout period in minutes.
747+
*
748+
* The value must be between 5 and 1440 minutes.
749+
*
750+
* You must specify one of `timeout` and `timeoutDurationMinutes`.
751+
*
752+
* @default - The value in `timeoutDurationMinutes` is used
736753
*/
737-
readonly timeoutDurationMinutes: number;
754+
readonly timeout?: cdk.Duration;
738755
}
739756

740757
/**

‎packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts

+9-3
Original file line numberDiff line numberDiff line change
@@ -150,14 +150,20 @@ function SpotProvisioningSpecificationPropertyToJson(property?: EmrCreateCluster
150150
if (!property) {
151151
return undefined;
152152
}
153-
if (!cdk.Token.isUnresolved(property.timeoutDurationMinutes) && (property.timeoutDurationMinutes < 5 || property.timeoutDurationMinutes > 1440)) {
154-
throw new Error(`timeoutDurationMinutes must be between 5 and 1440, got ${property.timeoutDurationMinutes}`);
153+
154+
if ((property.timeout && property.timeoutDurationMinutes) || (!property.timeout && !property.timeoutDurationMinutes)) {
155+
throw new Error('one of timeout and timeoutDurationMinutes must be specified');
156+
}
157+
const timeout = property.timeout?.toMinutes() ?? property.timeoutDurationMinutes;
158+
if (timeout !== undefined && !cdk.Token.isUnresolved(timeout) && (timeout < 5 || timeout > 1440)) {
159+
throw new Error(`timeout must be between 5 and 1440 minutes, got ${timeout} minutes.`);
155160
}
161+
156162
return {
157163
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy),
158164
BlockDurationMinutes: cdk.numberToCloudFormation(property.blockDurationMinutes),
159165
TimeoutAction: cdk.stringToCloudFormation(property.timeoutAction?.valueOf()),
160-
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeoutDurationMinutes),
166+
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeout?.toMinutes() || property.timeoutDurationMinutes),
161167
};
162168
}
163169

‎packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts

+118-6
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,7 @@ test.each([
921921
allocationStrategy: strategy,
922922
blockDurationMinutes: 1,
923923
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
924-
timeoutDurationMinutes: 5,
924+
timeout: cdk.Duration.minutes(5),
925925
},
926926
},
927927
name: 'Main',
@@ -1033,7 +1033,7 @@ test('Create Cluster with InstanceFleet for Spot instances', () => {
10331033
spotSpecification: {
10341034
blockDurationMinutes: 1,
10351035
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
1036-
timeoutDurationMinutes: 5,
1036+
timeout: cdk.Duration.minutes(5),
10371037
},
10381038
},
10391039
name: 'Main',
@@ -1219,7 +1219,63 @@ test('Create Cluster with InstanceFleet for On-Demand instances', () => {
12191219
});
12201220
});
12211221

1222-
test('Throws if timeoutDurationMinutes for Spot instances is less than 5', () => {
1222+
test('Throws if timeout for Spot instances is less than 5 minutes', () => {
1223+
// GIVEN
1224+
const task = new EmrCreateCluster(stack, 'Task', {
1225+
instances: {
1226+
instanceFleets: [{
1227+
instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
1228+
launchSpecifications: {
1229+
spotSpecification: {
1230+
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
1231+
timeout: cdk.Duration.minutes(4),
1232+
},
1233+
},
1234+
name: 'Main',
1235+
targetSpotCapacity: 1,
1236+
}],
1237+
},
1238+
clusterRole,
1239+
name: 'Cluster',
1240+
serviceRole,
1241+
integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
1242+
});
1243+
1244+
// THEN
1245+
expect(() => {
1246+
task.toStateJson();
1247+
}).toThrow(/timeout must be between 5 and 1440 minutes, got 4/);
1248+
});
1249+
1250+
test('Throws if timeout for Spot instances is greater than 1440 minutes', () => {
1251+
// GIVEN
1252+
const task = new EmrCreateCluster(stack, 'Task', {
1253+
instances: {
1254+
instanceFleets: [{
1255+
instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
1256+
launchSpecifications: {
1257+
spotSpecification: {
1258+
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
1259+
timeout: cdk.Duration.minutes(1441),
1260+
},
1261+
},
1262+
name: 'Main',
1263+
targetSpotCapacity: 1,
1264+
}],
1265+
},
1266+
clusterRole,
1267+
name: 'Cluster',
1268+
serviceRole,
1269+
integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
1270+
});
1271+
1272+
// THEN
1273+
expect(() => {
1274+
task.toStateJson();
1275+
}).toThrow(/timeout must be between 5 and 1440 minutes, got 1441 minutes./);
1276+
});
1277+
1278+
test('Throws if timeoutDurationMinutes for Spot instances is less than 5 minutes', () => {
12231279
// GIVEN
12241280
const task = new EmrCreateCluster(stack, 'Task', {
12251281
instances: {
@@ -1244,10 +1300,10 @@ test('Throws if timeoutDurationMinutes for Spot instances is less than 5', () =>
12441300
// THEN
12451301
expect(() => {
12461302
task.toStateJson();
1247-
}).toThrow(/timeoutDurationMinutes must be between 5 and 1440, got 4/);
1303+
}).toThrow(/timeout must be between 5 and 1440 minutes, got 4 minutes./);
12481304
});
12491305

1250-
test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440', () => {
1306+
test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440 minutes', () => {
12511307
// GIVEN
12521308
const task = new EmrCreateCluster(stack, 'Task', {
12531309
instances: {
@@ -1272,7 +1328,63 @@ test('Throws if timeoutDurationMinutes for Spot instances is greater than 1440',
12721328
// THEN
12731329
expect(() => {
12741330
task.toStateJson();
1275-
}).toThrow(/timeoutDurationMinutes must be between 5 and 1440, got 1441/);
1331+
}).toThrow(/timeout must be between 5 and 1440 minutes, got 1441 minutes./);
1332+
});
1333+
1334+
test('Throws if neither timeout nor timeoutDurationMinutes is specified', () => {
1335+
// GIVEN
1336+
const task = new EmrCreateCluster(stack, 'Task', {
1337+
instances: {
1338+
instanceFleets: [{
1339+
instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
1340+
launchSpecifications: {
1341+
spotSpecification: {
1342+
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
1343+
},
1344+
},
1345+
name: 'Main',
1346+
targetSpotCapacity: 1,
1347+
}],
1348+
},
1349+
clusterRole,
1350+
name: 'Cluster',
1351+
serviceRole,
1352+
integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
1353+
});
1354+
1355+
// THEN
1356+
expect(() => {
1357+
task.toStateJson();
1358+
}).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
1359+
});
1360+
1361+
test('Throws if both timeout and timeoutDurationMinutes are specified', () => {
1362+
// WHEN
1363+
const task = new EmrCreateCluster(stack, 'Task', {
1364+
instances: {
1365+
instanceFleets: [{
1366+
instanceFleetType: EmrCreateCluster.InstanceRoleType.MASTER,
1367+
launchSpecifications: {
1368+
spotSpecification: {
1369+
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
1370+
timeout: cdk.Duration.minutes(5),
1371+
timeoutDurationMinutes: 10,
1372+
},
1373+
},
1374+
name: 'Main',
1375+
targetSpotCapacity: 1,
1376+
}],
1377+
},
1378+
clusterRole,
1379+
name: 'Cluster',
1380+
serviceRole,
1381+
integrationPattern: sfn.IntegrationPattern.REQUEST_RESPONSE,
1382+
});
1383+
1384+
// THEN
1385+
expect(() => {
1386+
task.toStateJson();
1387+
}).toThrow(/one of timeout and timeoutDurationMinutes must be specified/);
12761388
});
12771389

12781390
test('Throws if both bidPrice and bidPriceAsPercentageOfOnDemandPrice are specified', () => {

0 commit comments

Comments
 (0)