-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat(stepfunctions-tasks): adding on demand option + allocation s… #12512
Changes from all commits
e5a8b5e
df49eb6
fe5ee19
ed2ef5c
3d51bc8
c824c2f
51c53df
d79f1a1
b9eb38a
d05c98e
b8e39b2
33fc397
89cf902
0a9d636
4d79453
e752d2a
dc8248a
f044f2e
a11a228
83d3dbd
39aa43b
ed6777b
7461d16
3c84890
3e236df
1d064d0
fcfd46f
7f67d2c
c0abdaa
cb50af5
85b2e57
019a51c
4378dde
c2c55da
9dc95ca
920d8f8
9fc2c2b
4dc0e24
b9848d5
dbaef03
37e0fbe
64d8ed1
057a1c7
949bd7e
37bc501
86c1257
4668797
9d8807e
f4e2db1
a61cf20
11e44b2
abec497
ad91ccc
940abd1
0375f4c
e74f61b
d873d51
9084d8a
6e2c031
58d395c
b4d8a0e
21a7d30
51de8a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -563,13 +563,48 @@ export namespace EmrCreateCluster { | |
} | ||
|
||
/** | ||
* The launch specification for Spot instances in the instance fleet, which determines the defined duration and provisioning timeout behavior. | ||
* Different allocation strategies that can be used by the EMR cluster | ||
*/ | ||
export class AllocationStrategy { | ||
/** lowest-price allocation strategy. */ | ||
public static readonly LOWEST_PRICE = new AllocationStrategy('lowest-price'); | ||
/** capacity-optimized allocation strategy */ | ||
public static readonly CAPACITY_OPTIMIZED = new AllocationStrategy('capacity-optimized'); | ||
|
||
/** | ||
* Create a new AllocationStrategy with an arbitrary allocation strategy. | ||
* | ||
* @param customAllocationStrategy the allocation strategy string, | ||
* for example "lowest-price" | ||
*/ | ||
Comment on lines
+574
to
+579
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are there any docs links we can also point to? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added link to the Cfn docs. |
||
public static of(customAllocationStrategy: string): AllocationStrategy { | ||
return new AllocationStrategy(customAllocationStrategy); | ||
} | ||
|
||
/** The name of the allocation strategy to be used in EMR task parameters */ | ||
public readonly name: string; | ||
|
||
private constructor(name: string) { | ||
this.name = name; | ||
} | ||
} | ||
|
||
/** | ||
* The launch specification for Spot Instances in the fleet, which determines the defined duration, provisioning timeout behavior, and allocation strategy. | ||
* | ||
* @see https://docs.aws.amazon.com/emr/latest/APIReference/API_SpotProvisioningSpecification.html | ||
* | ||
* @experimental | ||
*/ | ||
export interface SpotProvisioningSpecificationProperty { | ||
|
||
/** | ||
* Specifies the strategy to use in launching Spot Instance fleets. | ||
* | ||
* @default AllocationStrategy.CAPACITY_OPTIMIZED | ||
*/ | ||
readonly allocationStrategy?: AllocationStrategy; | ||
|
||
/** | ||
* The defined duration for Spot instances (also known as Spot blocks) in minutes. | ||
* | ||
|
@@ -589,19 +624,46 @@ export namespace EmrCreateCluster { | |
} | ||
|
||
/** | ||
* The launch specification for Spot instances in the fleet, which determines the defined duration and provisioning timeout behavior. | ||
* The launch specification for On-demand instances in the instance fleet, which determines the allocation strategy. | ||
* | ||
* @see https://docs.aws.amazon.com/emr/latest/APIReference/API_OnDemandProvisioningSpecification.html | ||
* | ||
* @experimental | ||
*/ | ||
export interface OnDemandProvisioningSpecificationProperty { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming, we generally use It was a miss and something we should clean up when we drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have followed existing conventions of using Property throughout. I think changing this to Props would require changing existing names to Props as well and that would introduce a breaking change. As for the experimental annotation, its used all over this file, is it fine to remove them all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, let's leave it alone for now. i'm not a fan of proliferating something we know we want to change because that's the way it's always been done. but i'm happy to do it in another change. it will break more things, but that's the only way to stop the trend. |
||
|
||
/** | ||
* Specifies the strategy to use in launching On-Demand Instance fleets. | ||
* | ||
* @default AllocationStrategy.LOWEST_PRICE | ||
*/ | ||
readonly allocationStrategy?: AllocationStrategy; | ||
} | ||
|
||
/** | ||
* The launch specification for Spot Instances in the fleet, which determines the defined duration, provisioning timeout behavior, and allocation strategy. | ||
* | ||
* @see https://docs.aws.amazon.com/emr/latest/APIReference/API_InstanceFleetProvisioningSpecifications.html | ||
* | ||
* @experimental | ||
*/ | ||
export interface InstanceFleetProvisioningSpecificationsProperty { | ||
/** | ||
* The launch specification for Spot instances in the fleet, which determines the defined duration and provisioning timeout behavior. | ||
* The launch specification for Spot Instances in the fleet, which determines the defined duration, provisioning timeout behavior, and allocation strategy. | ||
* | ||
* @default - None | ||
*/ | ||
readonly spotSpecification: SpotProvisioningSpecificationProperty; | ||
readonly spotSpecification?: SpotProvisioningSpecificationProperty; | ||
|
||
/** | ||
* The launch specification for On-Demand Instances in the instance fleet, which determines the allocation strategy. | ||
* | ||
* @default - None | ||
*/ | ||
readonly onDemandSpecification?: OnDemandProvisioningSpecificationProperty; | ||
} | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove extra line |
||
/** | ||
* The configuration that defines an instance fleet. | ||
* | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -115,18 +115,46 @@ export function InstanceTypeConfigPropertyToJson(property: EmrCreateCluster.Inst | |||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Render the SpotProvisioningSpecificationProperty to JSON | ||||||||||||
* | ||||||||||||
* @param property | ||||||||||||
*/ | ||||||||||||
export function SpotProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.SpotProvisioningSpecificationProperty) { | ||||||||||||
return { | ||||||||||||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy?.name), | ||||||||||||
BlockDurationMinutes: cdk.numberToCloudFormation(property.blockDurationMinutes), | ||||||||||||
TimeoutAction: cdk.stringToCloudFormation(property.timeoutAction?.valueOf()), | ||||||||||||
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.timeoutDurationMinutes), | ||||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Render the SpotProvisioningSpecificationProperty to JSON | ||||||||||||
* | ||||||||||||
* @param property | ||||||||||||
*/ | ||||||||||||
export function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty) { | ||||||||||||
return { | ||||||||||||
AllocationStrategy: cdk.stringToCloudFormation(property.allocationStrategy?.name), | ||||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
Comment on lines
+118
to
+142
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do both of these need to be in a utils file? - they're only used in one file. the preference is to keep it local until there's a need (multiple usage sites) to move it to a common location. |
||||||||||||
/** | ||||||||||||
* Render the InstanceFleetProvisioningSpecificationsProperty to JSON | ||||||||||||
* | ||||||||||||
* @param property | ||||||||||||
*/ | ||||||||||||
export function InstanceFleetProvisioningSpecificationsPropertyToJson(property: EmrCreateCluster.InstanceFleetProvisioningSpecificationsProperty) { | ||||||||||||
return { | ||||||||||||
SpotSpecification: { | ||||||||||||
BlockDurationMinutes: cdk.numberToCloudFormation(property.spotSpecification.blockDurationMinutes), | ||||||||||||
TimeoutAction: cdk.stringToCloudFormation(property.spotSpecification.timeoutAction?.valueOf()), | ||||||||||||
TimeoutDurationMinutes: cdk.numberToCloudFormation(property.spotSpecification.timeoutDurationMinutes), | ||||||||||||
}, | ||||||||||||
SpotSpecification: | ||||||||||||
property.spotSpecification === undefined | ||||||||||||
? property.spotSpecification | ||||||||||||
: SpotProvisioningSpecificationPropertyToJson(property.spotSpecification), | ||||||||||||
Comment on lines
+150
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be wrong, we don't want to convert to JSON if the field is undefined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't the function handle that? |
||||||||||||
OnDemandSpecification: | ||||||||||||
property.onDemandSpecification === undefined | ||||||||||||
? property.onDemandSpecification | ||||||||||||
: OnDemandProvisioningSpecificationPropertyToJson(property.onDemandSpecification), | ||||||||||||
Comment on lines
+154
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same question about the function. can it be more robust to return undefined if the property is undefined? |
||||||||||||
}; | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include a link to the docs - ideally we want to provide more information to users to determine which allocation is most appropriate for their use case(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, added link to the Cfn docs.