-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-26458 Update Snapshot TTL doc #3852
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
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
Good catch. Thanks for finding this and fix it.
I think in the release note, we'b better mention clearly about the broken behavior in the past, and how it is changed after this fix. At least this is a behavior change, we need to let users clearly know the side efffect.
🎊 +1 overall
This message was automatically generated. |
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.
+1, thanks @joswiatek
@joswiatek Since attached patches over the Jira no longer triggers QA, can you once create PRs for branch-2 and branch-1? Once we have the QA, all PRs will be merged and branch-2 change can be cherry-picked to 2.4 and 2.3 release branches.
+1 |
Oh wait, now I recall why client and server side have different default TTL values. |
SnapshotDescription is used at client side to store TTL value. Now, the default value is
Hence, by default
As per this proto message, default value of TTL for
@joswiatek No specifying TTL while creating snapshot should result in using default TTL as specified by Also, here is the SnapshotCleanerChore logic that would delete only snapshots with value > 0 and value < Long.MAX:
I am confident that the current logic should work fine for the case where client doesn't specify snapshot resulting into using TTL specified by config |
The only case that might require behaviour change is when user specifies TTL 0:
This snapshot would also be eligible to be deleted as per config If use specifies TTL value less than -1, then that snapshot will never be deleted based on TTL.
It's also important to note that SnapshotCleanerChore can be enabled/disabled using shell command: |
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.
- If client wants to create specific snapshot that should live forever regardless of
hbase.master.snapshot.ttl
config value, then they should specify TTL < -1. e.gsnapshot 'mytable', 'snapshot1234', {TTL => -2}
- If we don't want to enable TTL at entire cluster level, no need of any config change because the default value of
hbase.master.snapshot.ttl
is 0, which represents TTL Forever at server side.
I think we need these 2 points to be clearly mentioned in the HBase book.
As for the PR, we can't make this change because it requires changing proto and changing default value of proto is not compatible change for cluster already utilizing this feature.
I just tested on the latest HEAD of master branch. Configs:
Shell commands and respective outputs:
After 5sec, the snapshot was gone. Configs:
Shell commands and respective outputs:
Snapshot 's4' gets created with TTL 20 sec because of config After 20s, it was gone:
I think we have doc with details of server side configs Also, it's bit complex to resolve point |
Thank you for digging into this! I totally missed the protobuf component. It is great to see this is working on master HEAD. After reviewing these new protobuf details, I think the behavior where
On a branch-1 build, I do see snapshots being created with a -1 TTL when the default config is set. Configs:
Shell commands and outputs
Snapshot is actually created with a TTL of
I am not experienced with protobuf, so I will take your word about defaults here. One question: is changing the logic around building the protobuf also an incompatible change? If we can update the TTL conversion logic to allow for sending If that's not possible, I can update this PR with the clarifying doc changes as suggested. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
Left minor comment, +1 otherwise.
I've included the additional doc suggestions, thanks for the review y'all |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks @joswiatek. Left one comment on #3857 for branch-1 backport. Once done, will merge both PRs. |
See JIRA for detail: https://issues.apache.org/jira/browse/HBASE-26458
The bug is that the default value for TTL when no explicit TTL is -1, but the snapshot validation that applies
hbase.master.snapshot.ttl
looks for0
. Both should use -1, and share the value in a constant.