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 Slot Validation for Custom Proc #712

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

vazois
Copy link
Contributor

@vazois vazois commented Oct 9, 2024

This PR fixes slot verification for custom proc.
It introduces the following:

  • Iterative slot verification interface.
  • Slot verification fix for WATCH command.
  • Separation of WATCH_MS and WATCH_OS as separate commands instead of WATCH subcommands
  • Added BDN for custom proc

RespClusterBench

Method Mean Error StdDev Allocated
Get 22.29 us 0.025 us 0.022 us -
Set 20.67 us 0.030 us 0.027 us -
MGet 17.57 us 0.020 us 0.018 us -
MSet 16.44 us 0.018 us 0.016 us -
CPBSET 171.82 us 0.333 us 0.311 us 8192 B

RespClusterDisableSlotVerificationBench

Method Mean Error StdDev Allocated
Get 18.18 us 0.028 us 0.026 us -
Set 18.33 us 0.076 us 0.071 us -
MGet 16.82 us 0.029 us 0.028 us -
MSet 15.05 us 0.054 us 0.051 us -
CPBSET 153.72 us 0.328 us 0.307 us 8192 B

@vazois vazois force-pushed the vazois/custom-proc-cluster branch 4 times, most recently from e52db1e to 6740fc8 Compare October 11, 2024 22:19
@@ -172,6 +175,13 @@ internal bool RunTransactionProc(byte id, ArgSlice input, CustomTransactionProce
return false;
}

if (state == TxnState.Aborted)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if there should another state (CROSSLOTABORT?) when slot verification fails.

Copy link
Contributor Author

@vazois vazois Oct 15, 2024

Choose a reason for hiding this comment

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

It is not difficult to add but how is this useful? When txn is aborted the client will receive a message that validation failed due slot verification issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, transaction fail possible only due to slot verification issue. Can there be more reasons later? If yes, it would keep showing slot verification issue hence thought of another state.

@vazois vazois marked this pull request as ready for review October 15, 2024 18:03
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.

3 participants