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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lock acquisition): introduce new error type and retry policy for lock acquisition #329

Merged
merged 1 commit into from
Mar 24, 2024

Conversation

danielchalef
Copy link
Member

@danielchalef danielchalef commented Mar 24, 2024

馃殌 This PR description was created by Ellipsis for commit f76c55a.

Summary:

This PR introduces a new error type for lock acquisition failures, includes the failsafe-go package for retry policies, and updates memorystore.go and userstore.go to handle lock acquisition with a retry policy.

Key points:

  • Added AdvisoryLockError in /pkg/models/errors.go
  • Included failsafe-go package in go.mod and go.sum
  • Added tryAcquireAdvisoryLock function in /pkg/store/postgres/memorystore.go
  • Updated userstore.go to use failsafe.Get with a retry policy for lock acquisition

Generated with 鉂わ笍 by ellipsis.dev

@ellipsis-dev ellipsis-dev bot changed the title ... feat(lock acquisition): introduce new error type and retry policy for lock acquisition Mar 24, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

馃憤 Looks good to me!

  • Reviewed the entire pull request up to f76c55a
  • Looked at 172 lines of code in 5 files
  • Took 42 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. /pkg/store/postgres/userstore.go:106:
  • Assessed confidence : 0%
  • Comment:
    The use of the 'failsafe-go' library for retrying the lock acquisition seems appropriate. The retry policy is set to handle 'models.ErrLockAcquisitionFailed' errors, with a delay of 200 milliseconds and a maximum of 3 retries. This should help in situations where the lock cannot be immediately acquired. The type of the returned value from the 'failsafe.Get' function is also correctly checked.
  • Reasoning:
    The PR introduces a new library 'failsafe-go' for retrying the lock acquisition. The library is used in the 'Update' function of the 'UserStoreDAO' struct. The retry policy is set to handle 'models.ErrLockAcquisitionFailed' errors, with a delay of 200 milliseconds and a maximum of 3 retries. This seems to be a reasonable approach to handle situations where the lock cannot be immediately acquired. The code also correctly checks the type of the returned value from the 'failsafe.Get' function. The rest of the changes in the PR seem to be related to the introduction of this retry mechanism and do not introduce any new logic or modify existing one significantly. Therefore, I don't see any clear violations of best practices, logical bugs, performance bugs, or security bugs.

Workflow ID: wflow_SmN5iGiWhoA3jLmM


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

@danielchalef danielchalef merged commit b3f4b83 into main Mar 24, 2024
5 checks passed
@danielchalef danielchalef deleted the fix/metadata-deadlocks branch March 24, 2024 23:01
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

2 participants