-
Notifications
You must be signed in to change notification settings - Fork 752
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
Feature: transaction api #5030
Feature: transaction api #5030
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
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.
Not yet reviewed the execution part. As having some question about the datatype definition.
message TxnKey { | ||
string key = 1; | ||
string tenant = 2; | ||
string tree = 3; | ||
} |
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.
tenant
should be part of the key
IMHO.
Txn is only meant to be used on the sled tree kv
thus we may not need to specify a tree
.
The TxnKey
could be a simple string
. Or I missed something?
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.
is it possible that we may introduce some features like tenant isolation inside the metasrv in the future? if so, a metadata about tenent inside each key maybe helpful.
likewise we have an JWT inside the user session, each JWT is associated with a limitted scope of JWT, we may pass the JWT alongside to the metasrv, and metasrv can restrict the session can only access the data inside one tenant.
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.
is it possible that we may introduce some features like tenant isolation inside the metasrv in the future? if so, a metadata about tenent inside each key maybe helpful.
It can be done. But if to add this feature is still up to the design principle:
Whether to keep the metasrv a pure kv service like etcd or it has to understand some of the business logic.
Currently tenant
is embedded into the key
thus metasrv do not need to understand tenant
. Are we gonna break this rule?
message DeleteResponse { | ||
TxnKey key = 1; | ||
bool success = 2; | ||
optional SeqV prev_value = 3; | ||
} |
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.
Will there be an unsuccess delete? If the txn
conditions are not satisfied, the txn should not run at all. Otherwise, delete should always succeed.
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.
i just follow etcd api define, how about let transaction API user to judge these param
may be user get
in condition use another key, so i think notify if or not success if fine:)
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.
Is there any progress?
message DeleteResponse { | ||
TxnKey key = 1; | ||
bool success = 2; | ||
optional SeqV prev_value = 3; | ||
} |
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.
Is there any progress?
common/meta/types/proto/meta.proto
Outdated
message TxnReply { | ||
bool success = 1; | ||
repeated TxnOpResponse responses = 2; | ||
} |
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 a thought: if txn is aborted, the client may need to know which condition is not satisfied?
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
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.
💯
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
Summary
Feature: transaction api
Changelog
Related Issues
Fixes #4918