-
Notifications
You must be signed in to change notification settings - Fork 0
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 fatal error #20
Fix fatal error #20
Conversation
sisygoboom
commented
May 30, 2024
- improve logging for future debugging
- error handling in lock service
- remove unused code in lock service
All deleted code is lefto over from a switch in how locking is performed. formerly the nodes would constantly switch, now they remain the primary node until there's a failure. execute with lock catches the failure and unregisters the node leaving it open to another node to take |
|
||
// Optionally, you can decide if the lock should be released immediately after the task | ||
// or if it should be held until the next execution cycle. | ||
// await this.unregisterLock(lockId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't await this.unregisterLock(lockId);
required before this function returns to unlock the taskId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its optional, I foresee fewer problems keeping things on one node until task execution failure and then falling back vs constantly switching but if you disagree I'd appreciate your input
async registerSelf() { | ||
const dist1 = distance(Buffer.from('foo'), Buffer.from('bar')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking, these aren't used anywhere else, right? They are public functions after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the entirety of the lock service is now accessed via the execute with lock method. They're public because of legacy from when the lock service was accessed in a non-nest/mnon-modular style.
await this.lockRepository.findOneByIdAndMakeInactiveIfExists({ id }); | ||
|
||
// Option 2: Delete the lock from the repository if locks are treated as ephemeral | ||
// await this.lockRepository.deleteLock(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have been commented out? Looks important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we mark this node as inactive on line 137, meaning other nodes can register themselves. Deleting and marking things as inactive serve the same purpose here
Ah I didn't see this when I was reviewing. I understand about this now. Feel free to resolve comments about deleted code. Just do double check that the deleted functions are not used elsewhere in the codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM