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

Shutdown processor and worker on init fail #7654

Merged

Conversation

pradovic
Copy link
Contributor

@pradovic pradovic commented Feb 26, 2024

Current behavior

Worker and Processor do not shutdown if initialize function fails, per #7575.

Proposed changes

If the initialize function fail, do not proceed to run loops, but instead shutdown self and stop ack the ctx.

@adrianbenavides
Copy link
Member

Thanks for submitting this PR @pradovic! It looks good but we need to merge the CLA PR first before we can approve this one.

@adrianbenavides
Copy link
Member

adrianbenavides commented Mar 8, 2024

Hey @pradovic, we need you to rebase your branch on top of latest develop to grab your CLA changes and pass the lint_commits CI check.

Also, we saw here there is a problem with one test. To fix it, you need to change the sleep time as I did here and here. I tried to push the changes myself, but for some reason they didn't get through.

@pradovic pradovic force-pushed the worker-processor-init-shutdown branch from 919cf50 to 8b28fd5 Compare March 8, 2024 12:04
@pradovic
Copy link
Contributor Author

pradovic commented Mar 8, 2024

Hey @pradovic, we need you to rebase your branch on top of latest develop to grab your CLA changes and pass the lint_commits CI check.

Also, we saw here there is a problem with one test. To fix it, you need to change the sleep time as I did here and here. I tried to push the changes myself, but for some reason they didn't get through.

Thanks for the review! I rebased with develop and added a sleep time.

@pradovic pradovic force-pushed the worker-processor-init-shutdown branch 3 times, most recently from ba052f6 to db9bc60 Compare March 14, 2024 11:49
@SanjoDeundiak
Copy link
Member

@pradovic thank you for submitting the PR! I noticed that we needed to increase the sleep time on a test to make it work again. That makes me a little bit worried that there is a bug in our code which wasn't visible before. I think your code is absolutely fine, but I'll need to inspect how it affected that test before we can merge your PR. That will take a while. I'll get back to you as soon as I can

@pradovic
Copy link
Contributor Author

@SanjoDeundiak Ok, thanks! let me know if I can help, I am trying to reproduce it locally but did not manage to do it. I will try to take a look as well.

@SanjoDeundiak SanjoDeundiak force-pushed the worker-processor-init-shutdown branch from db9bc60 to c2e041c Compare April 3, 2024 14:43
@adrianbenavides adrianbenavides force-pushed the worker-processor-init-shutdown branch 3 times, most recently from aff06ae to 97310a9 Compare April 15, 2024 14:38
SanjoDeundiak
SanjoDeundiak previously approved these changes Apr 15, 2024
@SanjoDeundiak SanjoDeundiak added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2024
@adrianbenavides adrianbenavides added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2024
@adrianbenavides adrianbenavides added this pull request to the merge queue Apr 16, 2024
Merged via the queue into build-trust:develop with commit d76728e Apr 16, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants