Skip to content

Commit

Permalink
refactor: do not use TxnPutRequest|TxnDeleteRequest.prev_value, alw…
Browse files Browse the repository at this point in the history
…ays return previous value (databendlabs#14371)

Meta-service does not check `TxnPutRequest.prev_value` and
`TxnDeleteRequest.prev_value` any more, and always assumes it is true.
When a `Put` or `Delete` request is executed, the previous value before
`Put` will always be responded.
  • Loading branch information
drmingdrmer authored Jan 18, 2024
1 parent ff5f657 commit 8cb4acc
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 43 deletions.
124 changes: 107 additions & 17 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/meta/client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ pub static METACLI_COMMIT_SEMVER: LazyLock<Version> = LazyLock::new(|| {
/// client: remove using MetaGrpcReq::GetKV/MGetKV/ListKV;
/// client: remove falling back kv_read_v1(Streamed(List)) to kv_api(List), added in `2023-10-20: since 1.2.176`;
///
/// - 2024-01-17: since TODO:
/// server: do not use TxnPutRequest.prev_value;
/// server: do not use TxnDeleteRequest.prev_value;
/// Always return the previous value;
/// field index is reserved, no compatibility changes.
///
/// Server feature set:
/// ```yaml
/// server_features:
Expand Down
12 changes: 2 additions & 10 deletions src/meta/raft-store/src/applier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,7 @@ impl<'a> Applier<'a> {

let put_resp = TxnPutResponse {
key: put.key.clone(),
prev_value: if put.prev_value {
prev.map(pb::SeqV::from)
} else {
None
},
prev_value: prev.map(pb::SeqV::from),
};

resp.responses.push(TxnOpResponse {
Expand Down Expand Up @@ -428,11 +424,7 @@ impl<'a> Applier<'a> {
let del_resp = TxnDeleteResponse {
key: delete.key.clone(),
success: is_deleted,
prev_value: if delete.prev_value {
prev.map(pb::SeqV::from)
} else {
None
},
prev_value: prev.map(pb::SeqV::from),
};

resp.responses.push(TxnOpResponse {
Expand Down
12 changes: 2 additions & 10 deletions src/meta/raft-store/src/state_machine/sm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,11 +549,7 @@ impl StateMachine {

let put_resp = TxnPutResponse {
key: put.key.clone(),
prev_value: if put.prev_value {
prev.map(pb::SeqV::from)
} else {
None
},
prev_value: prev.map(pb::SeqV::from),
};

resp.responses.push(TxnOpResponse {
Expand Down Expand Up @@ -590,11 +586,7 @@ impl StateMachine {
let del_resp = TxnDeleteResponse {
key: delete.key.clone(),
success: is_deleted,
prev_value: if delete.prev_value {
prev.map(pb::SeqV::from)
} else {
None
},
prev_value: prev.map(pb::SeqV::from),
};

resp.responses.push(TxnOpResponse {
Expand Down
3 changes: 3 additions & 0 deletions src/meta/types/proto/request.proto
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ message TxnPutRequest {
bytes value = 2;

// Whether or not to return the prev value
// Not used anymore
bool prev_value = 3;

// Absolute expire time
Expand All @@ -64,7 +65,9 @@ message TxnPutResponse {
// Delete request and response
message TxnDeleteRequest {
string key = 1;

// if or not return the prev value
// Not used anymore
bool prev_value = 2;

// Delete only if the `seq` matches the specified value.
Expand Down
14 changes: 8 additions & 6 deletions src/meta/types/src/proto_display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

use std::fmt::Display;
use std::fmt::Formatter;
use std::time::Duration;
use std::time::SystemTime;

use num_traits::FromPrimitive;

Expand Down Expand Up @@ -144,13 +146,13 @@ impl Display for TxnGetRequest {

impl Display for TxnPutRequest {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(
f,
"Put key={}, need prev_value: {}",
self.key, self.prev_value
)?;
write!(f, "Put key={}", self.key)?;
if let Some(expire_at) = self.expire_at {
write!(f, " expire at: {}", expire_at)?;
let t = SystemTime::UNIX_EPOCH + Duration::from_millis(expire_at);
write!(f, " expire_at: {:?}", t)?;
}
if let Some(ttl_ms) = self.ttl_ms {
write!(f, " ttl: {:?}", Duration::from_millis(ttl_ms))?;
}
Ok(())
}
Expand Down

0 comments on commit 8cb4acc

Please sign in to comment.