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

Throw specific errors when there's an issue either acquiring or releasing the lock #284

Open
vany0114 opened this issue Aug 11, 2023 · 1 comment

Comments

@vany0114
Copy link

vany0114 commented Aug 11, 2023

Hi @mike-marcacci, with this new version, what would be the right way to handle errors acquiring a lock as well as errors releasing it? In the previous version, it used to return specific errors which is the right way because it allows consumers to handle them properly as needed.

  • Acquiring a lock error: Exceeded 10 attempts to lock the resource 'x'.
  • Releasing the lock error: Unable to fully release the lock on resource 'x'.

I noticed this version doesn't throw specific errors anymore in the scenarios described above, it always throws this generic error: The operation was applied to: 0 of the 1 requested resources. and actually it's not even thrown, it's just emitted.

As a consumer of the library, it is super important to be able to handle specific errors so that we can handle them properly, for instance, to build a proper resilience strategy, since for example, errors acquiring a lock are more critical and subject to be retried than errors releasing the lock which are subject to be ignored.

@pseudoralph
Copy link

@vany0114 I also share your concern— throwing a ResourceLockederror when a vote goes against for a given instance and then handling such errors as this more generic ExecutionError is less than ideal.

I was playing around w/ adding this to the _execute event loop

        while (true) {
            const { vote, stats } = await this._attemptOperation(script, keys, args);
            attempts.push(stats);
            // The operation achieved a quorum in favor.
            if (vote === "for") {
                return { attempts };
            }
            if (vote === 'against') {
                throw new ResourceLockedError("Quorum Achieved. This instance lost vote")
            }

re-throwing as ResourceLockedError (which is also an exported class of this module) would then allow the consumer of this package to ignore these types of expected errors and only throw for other, unhandled errors.

@mike-marcacci is this something I should create a PR for? i do notice a large backlog of unreviewed PRs and am wondering if this would be a welcome addition

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

No branches or pull requests

2 participants