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

AtomicWriteOutput.failed_checks is not populated when AtomicWrite checks fail #110

Open
h4l opened this issue Jan 30, 2025 · 0 comments
Open

Comments

@h4l
Copy link

h4l commented Jan 30, 2025

Judging by this todo comment in the code, this is a somewhat known issue, but I just wanted to record it explicitly here.

I've been implementing a DenoKV client in Python, and I found that the self-hosted implementation does not indicate which checks failed when it sends an AtomicWriteOutput with status AW_CHECK_FAILURE. The cloud implementation does report the failed_checks indexes.

I was interpreting the spec as meaning the server would be required to send failed_checks, but reading it again now I can read it as not explicitly requiring the server to send them.

https://github.com/denoland/denokv/blob/main/proto/kv-connect.md#atomic-write-request

[...] If a write operation fails due to a check conflict, the status field MUST be set to AW_CHECK_FAILED. [...]

(Nothing about requiring the server to use check_failures.)

If the response has a status field set to AW_CHECK_FAILED, the client MUST return an error to the user indicating that the write operation failed due to a check conflict. The client SHOULD report the checks that failed to the user by interpereting the check_failures field of the response.

I was treating no check indexes as an invalid response, but I guess I'll loosen that up and make no check indexes mean "don't know which checks failed".

FWIW it would be useful to have the indexes, because it would allow for re-reading just the outdated keys in order to retry automatically.

h4l added a commit to h4l/denokv-python that referenced this issue Feb 2, 2025
The denokv self-hosted implementation does not return indexes of failed
checks, so we need to allow this. The spec does not say that servers
MUST report indexes of failed checks, only that clients SHOULD report
failed indexes. See: denoland/denokv#110
h4l added a commit to h4l/denokv-python that referenced this issue Feb 3, 2025
The denokv self-hosted implementation does not return indexes of failed
checks, so we need to allow this. The spec does not say that servers
MUST report indexes of failed checks, only that clients SHOULD report
failed indexes. See: denoland/denokv#110
h4l added a commit to h4l/denokv-python that referenced this issue Feb 4, 2025
The denokv self-hosted implementation does not return indexes of failed
checks, so we need to allow this. The spec does not say that servers
MUST report indexes of failed checks, only that clients SHOULD report
failed indexes. See: denoland/denokv#110
h4l added a commit to h4l/denokv-python that referenced this issue Feb 4, 2025
The denokv self-hosted implementation does not return indexes of failed
checks, so we need to allow this. The spec does not say that servers
MUST report indexes of failed checks, only that clients SHOULD report
failed indexes. See: denoland/denokv#110
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

1 participant