Skip to content

Conversation

@habc12
Copy link
Contributor

@habc12 habc12 commented Feb 2, 2022

This PR has two changes:

  • Automatically run deployments for the coordinator/worker when the coordinator/worker config map changes.
  • Separated jvm args & resource values for the coordinator and worker.

@cla-bot
Copy link

cla-bot bot commented Feb 2, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12 habc12 changed the title Separate worker/coordinator config Separate worker/coordinator values Feb 2, 2022
@cla-bot
Copy link

cla-bot bot commented Feb 3, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

2 similar comments
@cla-bot
Copy link

cla-bot bot commented Feb 3, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link

cla-bot bot commented Feb 3, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Some questions, looks good otherwise.

@hashhar hashhar changed the title Separate worker/coordinator values Allow to provide different JVM configs for co-ordinator and workers Feb 4, 2022
@hashhar
Copy link
Member

hashhar commented Feb 4, 2022

Please squash all commits (since it's single logical change) with a message like the PR title.

@habc12 habc12 changed the title Allow to provide different JVM configs for co-ordinator and workers Provide different JVM configs for co-ordinator and workers Feb 4, 2022
@cla-bot
Copy link

cla-bot bot commented Feb 4, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12 habc12 force-pushed the habc12-separate-coordinator-worker-values branch from 6b275f5 to b470089 Compare February 4, 2022 19:54
@cla-bot
Copy link

cla-bot bot commented Feb 4, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@hashhar
Copy link
Member

hashhar commented Feb 7, 2022

@habc12 Please take a look at CI failure.

@cla-bot
Copy link

cla-bot bot commented Feb 8, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Feb 8, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12 habc12 force-pushed the habc12-separate-coordinator-worker-values branch from cba3c71 to c13ea03 Compare February 8, 2022 19:12
@cla-bot
Copy link

cla-bot bot commented Feb 8, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12
Copy link
Contributor Author

habc12 commented Feb 8, 2022

@habc12 Please take a look at CI failure.

Fixed the CI failure. It was a lint issue, when testing the charts you need to create a ci dir with custom values to be picked up by ct.

@cla-bot
Copy link

cla-bot bot commented Feb 9, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12
Copy link
Contributor Author

habc12 commented Feb 9, 2022

Latest version has fixed the CI issue when I tested it locally.

@habc12 habc12 force-pushed the habc12-separate-coordinator-worker-values branch from a730d23 to 98837df Compare February 11, 2022 20:02
@cla-bot
Copy link

cla-bot bot commented Feb 11, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@habc12
Copy link
Contributor Author

habc12 commented Feb 15, 2022

I sent the signed cla. Please let me know if there are any issues or if it has not been received. Thanks

@hashhar
Copy link
Member

hashhar commented Feb 17, 2022

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Feb 17, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link

cla-bot bot commented Feb 17, 2022

The cla-bot has been summoned, and re-checked this pull request!

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks.

Can you rebase to resolve conflicts?

Copy link
Member

Choose a reason for hiding this comment

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

We need to trigger new deployments on ANY change, not just these two config maps.

I think existing behaviour where operator decides to do fresh deployment should be kept as it is for now.

We can add feature in separate PR to trigger deployment when any catalogs change or node configs change.

@hashhar
Copy link
Member

hashhar commented Feb 18, 2022

cc: @valeriano-manassero @ckdarby for review/input

@habc12 habc12 closed this Feb 18, 2022
@habc12 habc12 force-pushed the habc12-separate-coordinator-worker-values branch from bd472de to a933fc1 Compare February 18, 2022 19:20
@dragonslayer27
Copy link
Contributor

@hashhar I accidentally closed this PR, I created a new one without the deployment change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants