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: ✨ Added Lifecycle Hook and Termination Grace Period #94

Merged
merged 1 commit into from
May 10, 2024

Conversation

danielcrisap
Copy link
Contributor

  • Adding Lifecycle Hook
  • Added Termination Grace Period

Those features are intended to achieve the Graceful shutdown from Trino Worker nodes.
https://trino.io/docs/current/admin/graceful-shutdown.html#admin-graceful-shutdown

With lifecycle hooks, when the pods receive a SIGTERM will request the API to start the SHUTTING_DOWN state and the terminationGracePeriodSeconds is for K8s waiting for this process to be concluded.

@cla-bot
Copy link

cla-bot bot commented Aug 22, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@tbaeg
Copy link
Member

tbaeg commented Dec 7, 2023

+1 for this feature.

Would be a nice feature to have on the coordinator as well. I can branch this PR and create another one since I'm unsure when the CLA will be signed.

@mosabua
Copy link
Member

mosabua commented Dec 8, 2023

@cla-bot check

Copy link

cla-bot bot commented Dec 8, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Dec 8, 2023

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

@danielcrisap
Copy link
Contributor Author

Sorry for the delay,

I've sent the email with the CLA signed; I will wait a few days to recheck.

@tbaeg
Copy link
Member

tbaeg commented Dec 8, 2023

@danielcrisap Any chance you could also add these to the coordinator?

Copy link

cla-bot bot commented Dec 8, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@danielcrisap
Copy link
Contributor Author

@danielcrisap Any chance you could also add these to the coordinator?

I included the coordinator; I just removed the example because, in the Trino doc, this API is only for Worker nodes.

Trino has a graceful shutdown API that can be used exclusively on workers in order to ensure that they terminate without affecting running queries, given a sufficient grace period.

@mosabua
Copy link
Member

mosabua commented Dec 8, 2023

Well.. there is a graceful shutdown for coordinator in the works...

@tbaeg
Copy link
Member

tbaeg commented Dec 8, 2023

@danielcrisap Any chance you could also add these to the coordinator?

I included the coordinator; I just removed the example because, in the Trino doc, this API is only for Worker nodes.

Trino has a graceful shutdown API that can be used exclusively on workers in order to ensure that they terminate without affecting running queries, given a sufficient grace period.

Awesome, thank you! Yeah, mostly looking for the lifecycle option as it provides flexibility for some custom operations to be executed. Appreciate you including it!

Copy link

cla-bot bot commented Dec 9, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@danielcrisap
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Dec 18, 2023
Copy link

cla-bot bot commented Dec 18, 2023

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

@avinashdesireddy
Copy link

I'm interested in this feature, Can this be merged? Happy to support any additional changes here.

@danielcrisap
Copy link
Contributor Author

@tbaeg and @mosabua ,
Do you think we can include this feature in the next release?

@tbaeg
Copy link
Member

tbaeg commented Mar 30, 2024

No issues from me but I also have no decision making authority. :)

@mosabua
Copy link
Member

mosabua commented Apr 18, 2024

@radek-starburst @losipiuk @nineinchnick .. any chance you could look at this?

charts/trino/README.md Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/README.md Outdated Show resolved Hide resolved
Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

It looks ready to be merged.

Please squash all commits, see https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#git-merge-strategy for more info.

charts/trino/values.yaml Outdated Show resolved Hide resolved
@danielcrisap
Copy link
Contributor Author

It looks ready to be merged.

Please squash all commits, see https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#git-merge-strategy for more info.

I don't have merge permission, so I can't set to use the squad strategy for merging into main.

@nineinchnick
Copy link
Member

You can squash commits locally and force-push to this branch.

@danielcrisap
Copy link
Contributor Author

You can squash commits locally and force-push to this branch.

Squashed into a single commit ✅

@nineinchnick nineinchnick merged commit 50e5089 into trinodb:main May 10, 2024
6 checks passed
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.

None yet

5 participants