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.

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(rosetta): JSII_ROSETTA_MAX_WORKER_COUNT limits max workers #2816

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

njlynch
Copy link
Contributor

@njlynch njlynch commented Apr 27, 2021

The current behavior of rosetta (extract) is to use a number of node worker
threads equal to half the number of CPU cores. On large CI/CD build hardware,
this may mean creating a huge number of worker threads which causes more
contention than benefit.

Introduce JSII_ROSETTA_MAX_WORKER_COUNT to limit the maximum number of workers
(defaults to 16).

See aws/aws-cdk#14389 for more motivation from the AWS CDK.


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

@njlynch njlynch requested a review from a team April 27, 2021 10:53
@njlynch njlynch self-assigned this Apr 27, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 27, 2021
The current behavior of rosetta (extract) is to use a number of node worker
threads equal to half the number of CPU cores. On large CI/CD build hardware,
this may mean creating a huge number of worker threads which causes more
contention than benefit.

Introduce JSII_ROSETTA_MAX_WORKER_COUNT to limit the maximum number of workers
(defaults to 16).

See aws/aws-cdk#14389 for more motivation from the AWS CDK.
@njlynch njlynch force-pushed the njlynch/rosetta-worker-threads branch from 320cffd to b72c7dd Compare April 27, 2021 10:55
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

Minor tweak in the README.

packages/jsii-rosetta/README.md Outdated Show resolved Hide resolved
@RomainMuller RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label Apr 27, 2021
Co-authored-by: Romain Marcadier <rmuller@amazon.com>
@njlynch njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Apr 27, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Apr 27, 2021
@mergify mergify bot merged commit c478da4 into main Apr 27, 2021
@mergify mergify bot deleted the njlynch/rosetta-worker-threads branch April 27, 2021 12:28
@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Apr 27, 2021
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Jun 6, 2021
Our `yarn-upgrade` workflow is failing with:

```console
@lerna/package-graph@4.0.0 ✔

**ERROR** Failed to apply patch for package jsii-rosetta at path
  
    node_modules/jsii-rosetta

  This error was caused because jsii-rosetta has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.
```

Basically, its not able to apply the [rosetta patch](https://github.com/aws/aws-cdk/blob/master/patches/jsii-rosetta%2B1.28.0.patch) anymore since a new version of `jsii-rosetta` has been released which creates conflicts. 

The new version of rosetta [addresses](aws/jsii#2816) the problem the patch was trying to solve, so it is no longer needed (yey). 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
Our `yarn-upgrade` workflow is failing with:

```console
@lerna/package-graph@4.0.0 ✔

**ERROR** Failed to apply patch for package jsii-rosetta at path
  
    node_modules/jsii-rosetta

  This error was caused because jsii-rosetta has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.
```

Basically, its not able to apply the [rosetta patch](https://github.com/aws/aws-cdk/blob/master/patches/jsii-rosetta%2B1.28.0.patch) anymore since a new version of `jsii-rosetta` has been released which creates conflicts. 

The new version of rosetta [addresses](aws/jsii#2816) the problem the patch was trying to solve, so it is no longer needed (yey). 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants