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

Fix worker updates #2131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Viren6
Copy link

@Viren6 Viren6 commented Aug 17, 2024

Releases the worker lock before restarting for an update. Fixes #2130

@vondele
Copy link
Member

vondele commented Aug 17, 2024

So that's potentially an important thing to fix first.... will it be possible to apply this fix automatically, or will users need to do something by hand (like restart the worker). If the latter is the case, we might need to go in two steps (e.g. skip lock check in a first update, and only apply the full fix in a second step).

@Viren6
Copy link
Author

Viren6 commented Aug 17, 2024

I believe they would need to manually restart the worker. My workers are dockerised and I had to hard redeploy them to get them working.

@vondele
Copy link
Member

vondele commented Aug 17, 2024

so, that probably means we will have to apply the fix in two steps if that's possible.

@vdbergh
Copy link
Contributor

vdbergh commented Aug 17, 2024

I guess I messed up the updating process... I neglected to test it since I thought that it would work in the same way as before. But now I realize that there is a fundamental distinction between the original adhoc locking implementation and the refactored one. The former one was sort of reentrant and so it had no issues with updating.

@vdbergh
Copy link
Contributor

vdbergh commented Aug 17, 2024

so, that probably means we will have to apply the fix in two steps if that's possible.

It is possible I guess.

PR1: this PR+comment out locking statement in the worker.
PR2: comment in locking statements.

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.

Worker updates broken after refactored locking implementation
3 participants