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

Added a way to set a TTL to key values. #137

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Added a way to set a TTL to key values. #137

merged 2 commits into from
Mar 12, 2024

Conversation

fulmicoton
Copy link
Contributor

After the GC grace period, the key value will be subject to the GC just like regularly deleted KVs.

@fulmicoton fulmicoton marked this pull request as ready for review March 8, 2024 08:52
@fulmicoton fulmicoton requested a review from trinity-1686a March 8, 2024 08:54
Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

looks sensible; see comments

{
let versioned_value = node_state.get_versioned("key").unwrap();
assert_eq!(&versioned_value.value, "");
assert_eq!(versioned_value.version, 2u64);
assert!(&versioned_value.is_tombstone());
assert!(&versioned_value.is_deleted());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert!(&versioned_value.is_deleted());
assert!(versioned_value.is_deleted());

i don't think the borrow is necessary, and i find it somewhat confusing (is it supposed to be a borrow, or a mistyped !)

Copy link
Contributor Author

@fulmicoton fulmicoton Mar 11, 2024

Choose a reason for hiding this comment

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

yes, I find it confusing too.

@@ -108,7 +133,7 @@ impl PartialEq for VersionedValue {
fn eq(&self, other: &Self) -> bool {
self.value.eq(&other.value)
&& self.version.eq(&other.version)
&& self.is_tombstone().eq(&other.is_tombstone())
&& self.is_deleted().eq(&other.is_deleted())
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 not sure is_deleted is enough to qualify equality. DeletionStatus::Set and DeletionStatus::DeleteAfterTtl are considered equal by this statement. If that is intended, a comment should explain why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx. I switched to checking the discriminant, added a comment, and made PartialEq cfg test only.

After the GC grace period, the key value will be subject to the GC
just like regularly deleted KVs.
@fulmicoton fulmicoton merged commit de1edcf into main Mar 12, 2024
2 checks passed
@fulmicoton fulmicoton deleted the ttl3 branch March 12, 2024 02:28
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.

2 participants