feat(stepfunctions-tasks): EMR createCluster command support OnDemandSpecification by go-to-k · Pull Request #27791 · aws/aws-cdk

go-to-k · 2023-11-01T11:48:53Z

This PR supports OnDemandSpecification in instance fleets for EMR createCluster.

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

…Specification

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

go-to-k · 2023-11-02T11:15:08Z

This PR adds the integ tests for On-Demand only.

There is no integ test for Spot, but I added it in another PR.

#27809

lpizzinidev

Thanks!
The implementation looks correct.
Some adjustments are needed on the interface declaration in my opinion.

lpizzinidev · 2023-11-02T13:56:44Z

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

+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-emr-instancefleetconfig-ondemandprovisioningspecification.html
+   *
+   */
+  export interface OnDemandProvisioningSpecificationProperty {


This interface declaration is missing the CapacityReservationOptions attribute.

@lpizzinidev

I originally added it because it was in the API reference, but the CloudFormation document apparently does not support it, so I deleted it.

c0caee8#diff-f0a4ce25fba5b6013a9e99af978fe476abb485cdd149be1d30f20d7bd4f70591

Is this necessary?
In my opinion, it should follow CloudFormation, so I didn't think it was necessary to add it.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-emr-instancefleetconfig-ondemandprovisioningspecification.html

JSON { "AllocationStrategy" : String }

You're right, CFN is the right reference in this case.
Thanks for clarifying.

lpizzinidev · 2023-11-02T13:58:06Z

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

+     * Currently, the only option is lowest-price (the default), which launches the lowest price first.
+     *
+     * @default - lowest-price
+     */
+    readonly allocationStrategy?: OnDemandAllocationStrategy;


Suggested change

* Currently, the only option is lowest-price (the default), which launches the lowest price first.

*

* @default - lowest-price

*/

readonly allocationStrategy?: OnDemandAllocationStrategy;

* Currently, the only option is lowest-price (the default), which launches the lowest price first.

*/

readonly allocationStrategy: OnDemandAllocationStrategy;

The property is required.

Oh, I had been changed it to be possible for undefined because a default value is defined, but it is required according to CFn docs. So I will add!

I added them.

6510ab0

lpizzinidev · 2023-11-02T14:00:07Z

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

    /**
     * The launch specification for Spot instances in the fleet, which determines the defined duration and provisioning timeout behavior.
+     *
+     * @default - no spotSpecification


Suggested change

* @default - no spotSpecification

* @default - no spot specification

lpizzinidev · 2023-11-02T14:00:20Z

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

+     * The instance fleet configuration is available only in Amazon EMR releases 4.8.0 and later, excluding 5.0.x versions.
+     * On-Demand Instances allocation strategy is available in Amazon EMR releases 5.12.1 and later.
+     *
+     * @default - no onDemandSpecification


Suggested change

* @default - no onDemandSpecification

* @default - no on-demand specification

...t/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-on-demand-instance-fleet.ts

go-to-k · 2023-11-02T14:40:39Z

@lpizzinidev

Thanks again!

Please review the following things and changed codes!

#27791 (comment)

lpizzinidev

Thanks 👍

aws-cdk-automation · 2023-12-06T00:09:23Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

…sk-emr-ondemand

kaizencc

@go-to-k thanks for starting this! looks good, just a few minor stylistic comments!

kaizencc · 2023-12-18T18:37:25Z

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

+ *
+ * @param property


Suggested change

*

* @param property

kaizencc · 2023-12-18T18:37:41Z

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

+ *
+ * @param property
+ */
+function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty | undefined) {


Suggested change

function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty | undefined) {

function OnDemandProvisioningSpecificationPropertyToJson(property?: EmrCreateCluster.OnDemandProvisioningSpecificationProperty) {

kaizencc · 2023-12-18T18:42:20Z

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

+ *
+ * @param property


Suggested change

*

* @param property

If you want to leave an @param docstring, you have to describe the property like

@param property the spot provisioning specification

(But we don't need to do that here, I dont think)

kaizencc · 2023-12-18T18:44:14Z

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

+            allocationStrategy: EmrCreateCluster.OnDemandAllocationStrategy.LOWEST_PRICE,
+          },
+        },
+        name: 'Master',


Suggested change

name: 'Master',

name: 'Main',

(we're avoiding the term master now)

Pull request has been modified.

go-to-k · 2023-12-19T06:03:32Z

@kaizencc

Thanks for the review!

I fixed, so could you please take a look at them?

kaizencc

Thanks for changing the other instances of master to main as well. Appreciate it!

mergify · 2023-12-21T03:06:29Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-12-21T03:33:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8f34e1e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-21T03:33:34Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…Specification (aws#27791) This PR supports OnDemandSpecification in instance fleets for EMR createCluster. Closes aws#27761. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

go-to-k added 11 commits November 1, 2023 16:04

feat(stepfunctions-tasks): EMR createCluster command support OnDemand…

1a7a905

…Specification

change required to optional

97c9711

change tests and rendering

5fd3528

unit test

18d0ab7

wip in integ test

e3f4e40

test: add integ tests

b200d86

chore: add validation for length of capacityReservationResourceGroupArn

e235589

chore: tweak

56ada60

style: new line

904f7bf

chore: rendering props

8777b3c

change integ tests

f558357

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 1, 2023

aws-cdk-automation requested a review from a team November 1, 2023 11:49

github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Nov 1, 2023

aws-cdk-automation previously requested changes Nov 1, 2023

View reviewed changes

go-to-k added 6 commits November 1, 2023 21:05

test: change integ tests

5df65e7

chore: change params and tests

c0caee8

chore: rm cache

a3f29d2

chore: rm integs

b716028

chore: rm a line

4d5bd43

docs: change README

76dd650

go-to-k marked this pull request as ready for review November 2, 2023 09:26

go-to-k added 2 commits November 2, 2023 18:43

docs: fix codes in README

31a1726

docs: fix codes in README

c392bcf

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 2, 2023

lpizzinidev suggested changes Nov 2, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 2, 2023

go-to-k added 2 commits November 2, 2023 23:32

chore: allocationStrategy for on-demand to required

6510ab0

fix: by the review

18c92eb

go-to-k commented Nov 2, 2023

View reviewed changes

...t/aws-stepfunctions-tasks/test/emr/integ.emr-create-cluster-with-on-demand-instance-fleet.ts Show resolved Hide resolved

lpizzinidev approved these changes Nov 2, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 2, 2023

github-actions bot mentioned this pull request Dec 1, 2023

Monthly PR metrics report - November 2023 #28212

Closed

go-to-k and others added 5 commits December 6, 2023 14:25

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into sfn-ta…

7c660c8

…sk-emr-ondemand

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into sfn-ta…

dbe7c5d

…sk-emr-ondemand

use new module for apigatewayv2

40dcabc

Merge branch 'main' into sfn-task-emr-ondemand

956ecb1

Merge branch 'main' into sfn-task-emr-ondemand

4177095

kaizencc previously requested changes Dec 18, 2023

View reviewed changes

go-to-k added 2 commits December 19, 2023 13:44

modify by review

7c5a98e

Merge branch 'main' into sfn-task-emr-ondemand

68ca386

change snapshots

45bd660

go-to-k requested a review from kaizencc December 19, 2023 06:01

kaizencc approved these changes Dec 21, 2023

View reviewed changes

Merge branch 'main' into sfn-task-emr-ondemand

8f34e1e

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 21, 2023

mergify bot merged commit 73a5e74 into aws:main Dec 21, 2023

	* @default - no spotSpecification
	* @default - no spot specification

	* @default - no onDemandSpecification
	* @default - no on-demand specification

	function OnDemandProvisioningSpecificationPropertyToJson(property: EmrCreateCluster.OnDemandProvisioningSpecificationProperty \| undefined) {
	function OnDemandProvisioningSpecificationPropertyToJson(property?: EmrCreateCluster.OnDemandProvisioningSpecificationProperty) {

Conversation

go-to-k commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Nov 2, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

go-to-k Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

go-to-k commented Nov 2, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Dec 6, 2023

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 21, 2023

Uh oh!

aws-cdk-automation commented Dec 21, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

go-to-k commented Nov 1, 2023 •

edited

Loading

go-to-k Nov 2, 2023 •

edited

Loading

go-to-k commented Dec 19, 2023 •

edited

Loading