Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster #28532

Merged
merged 10 commits into from
Jan 2, 2024
Merged

feat(stepfunctions-tasks): add timeout parameter for EmrCreateCluster #28532

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ed0bb6e
feat(stepfunctions-tasks): add parameter as Duration type
go-to-k Dec 30, 2023
0e25bf3
Merge branch 'main' into feat/sfn-tasks-emr-new-timeout
go-to-k Dec 30, 2023
e06ab39
change docs
go-to-k Dec 30, 2023
f3840cc
change branch condition
go-to-k Dec 30, 2023
45b4a1b
change unit tests
go-to-k Dec 30, 2023
ad29524
change by review
go-to-k Dec 31, 2023
c1b020b
Merge branch 'main' into feat/sfn-tasks-emr-new-timeout
go-to-k Dec 31, 2023
b42f6c8
Apply suggestions from code review
kaizencc Jan 2, 2024
6a8c4ae
Merge branch 'main' into feat/sfn-tasks-emr-new-timeout
kaizencc Jan 2, 2024
5b49248
Merge branch 'main' into feat/sfn-tasks-emr-new-timeout
mergify[bot] Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...est/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-spot-instance-fleet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-stepfunctions-tasks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
},
}],
Expand Down
19 changes: 17 additions & 2 deletions packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/emr-create-cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,9 +732,24 @@ 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.
*
* @default - No timeoutDurationMinutes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default should be to use the value in timeout.

*
* @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 - No timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default should be that we use the value in timeoutDurationMinutes

*/
readonly timeoutDurationMinutes: number;
readonly timeout?: cdk.Duration;
}

/**
Expand Down
19 changes: 16 additions & 3 deletions packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/emr/private/cluster-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,27 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like treating timeout and timeoutDurationMinutes separately here, because that will cause difficulty in the future if we are to add more validation. Instead, I'm looking for:

if ((property.timeout && property.timeoutDurationMinutes) || (!property.timeout && !property.timeoutDurationMinutes)) { throw new Error() }
const timeout = property.timeout.toMinutes() ?? property.timeoutDurationMinutes;
if (!cdk.Token.isUnresolved(timeout) && timeout < 5 || timeout > 1440) { ... }

throw new Error('timeout must be specified');
}
if (property.timeout && (property.timeout.toMinutes() < 5 || property.timeout.toMinutes() > 1440)) {
throw new Error(`timeout must be between 5 and 1440 minutes, got ${property.timeout.toMinutes()}`);
}
if (
!property.timeout // ignore validation because `timeoutDurationMinutes` is not used if a `timeout` is specified
&& property.timeoutDurationMinutes
&& !cdk.Token.isUnresolved(property.timeoutDurationMinutes)
&& (property.timeoutDurationMinutes < 5 || property.timeoutDurationMinutes > 1440)
) {
throw new Error(`timeoutDurationMinutes must be between 5 and 1440 minutes, got ${property.timeoutDurationMinutes}`);
}

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),
};
}

Expand Down
161 changes: 155 additions & 6 deletions packages/aws-cdk-lib/aws-stepfunctions-tasks/test/emr/emr-create-cluster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ test.each([
allocationStrategy: strategy,
blockDurationMinutes: 1,
timeoutAction: EmrCreateCluster.SpotTimeoutAction.TERMINATE_CLUSTER,
timeoutDurationMinutes: 5,
timeout: cdk.Duration.minutes(5),
},
},
name: 'Main',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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/);
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
});

test('Throws if timeoutDurationMinutes for Spot instances is less than 5 minutes', () => {
// GIVEN
const task = new EmrCreateCluster(stack, 'Task', {
instances: {
Expand All @@ -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(/timeoutDurationMinutes must be between 5 and 1440 minutes, got 4/);
});

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: {
Expand All @@ -1272,7 +1328,100 @@ 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(/timeoutDurationMinutes must be between 5 and 1440 minutes, got 1441/);
});

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(/timeout must be specified/);
});

test('timeout takes precedence 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(stack.resolve(task.toStateJson())).toEqual({
Type: 'Task',
Resource: {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':states:::elasticmapreduce:createCluster',
],
],
},
End: true,
Parameters: {
Name: 'Cluster',
Instances: {
KeepJobFlowAliveWhenNoSteps: true,
InstanceFleets: [{
InstanceFleetType: 'MASTER',
LaunchSpecifications: {
SpotSpecification: {
TimeoutAction: 'TERMINATE_CLUSTER',
TimeoutDurationMinutes: 5,
},
},
Name: 'Main',
TargetSpotCapacity: 1,
}],
},
VisibleToAllUsers: true,
JobFlowRole: {
Ref: 'ClusterRoleD9CA7471',
},
ServiceRole: {
Ref: 'ServiceRole4288B192',
},
},
});
});

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