HBASE-22648 : Introducing Snapshot TTL #371
Conversation
virajjasani
commented
Jul 10, 2019
•
virajjasani
commented
- Associate TTL in sec with Snapshot argument. e.g. snapshot 'table', 'snapshot', {TTL => 20}
- If not specified, default TTL to be taken for Snapshot. Specified here: HConstants.DEFAULT_SNAPSHOT_TTL
- If Snapshot is never meant to get expired, specify negative value for TTL. snapshot 'table', 'snapshot', {TTL => -2}
| ttl > (Long.MAX_VALUE / 1000)) { | ||
| ttl = HConstants.DEFAULT_SNAPSHOT_TTL; | ||
| } | ||
| SnapshotDescription.Builder builder = snapshot.toBuilder(); |
There was a problem hiding this comment.
Should the snapshot build with ttl optional here, i.e. move to if clause above?
There was a problem hiding this comment.
The above if condition is to just update ttl. So regardless of whether we use default ttl or user provided ttl(ttl < -1 meaning never expiring ttl), we would have to build the snapshot anyways. Hence, kept this logic here.
Please let me know if this looks good
| try { | ||
| List<SnapshotProtos.SnapshotDescription> completedSnapshotsList = | ||
| this.snapshotManager.getCompletedSnapshots(); | ||
| if (CollectionUtils.isNotEmpty(completedSnapshotsList)) { |
There was a problem hiding this comment.
Not sure the if not-empty check is really needed since the for will skip if empty. It's not null.
| @InterfaceAudience.Private | ||
| public class SnapshotCleanerChore extends ScheduledChore { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(SnapshotCleanerChore.class); |
There was a problem hiding this comment.
nit: s/LOGGER/LOG/ if following other places.
| } | ||
| } | ||
| } catch (IOException e) { | ||
| LOGGER.error("Error while cleaning up Snapshots. Stopping Snapshot Cleaner Chore...", e); |
There was a problem hiding this comment.
"Stopping Snapshot Cleaner Chore..." might be confusing since it's still scheduled next time.
|
|
||
| @Override | ||
| protected void chore() { | ||
| LOGGER.trace("Snapshot Cleaner Chore is starting up..."); |
There was a problem hiding this comment.
Does it merit to add a config disabling this chore, in case of some data restore operation using about-to expire snapshots...
There was a problem hiding this comment.
What you are suggesting is adding a config to disable and restart HMaster(sort of EBF release) should disable running this chore as I can have a check here that only if the config is not disabled, perform all these steps?
Is that correct?
There was a problem hiding this comment.
Yes, exactly. Does it make sense?
There was a problem hiding this comment.
Definitely it's good to have disable config. Thanks @liuml07
| // set default ttl(sec) if it is not set already or the value is out of the range | ||
| if (ttl == SnapshotDescriptionUtils.NO_SNAPSHOT_TTL_SPECIFIED || | ||
| ttl > (Long.MAX_VALUE / 1000)) { | ||
| ttl = HConstants.DEFAULT_SNAPSHOT_TTL; |
There was a problem hiding this comment.
nit: add a LOG.debug() message here?
| EnvironmentEdgeManager.currentTime())); | ||
| Mockito.when(snapshotManager.getCompletedSnapshots()).thenReturn(snapshotDescriptionList); | ||
| try { | ||
| LOG.debug("2 Snapshots are completed but TTL is not expired for any of them"); |
|
@liuml07 Thanks for the review. Updated the patch |
| @@ -1445,6 +1445,9 @@ | |||
| "hbase.util.default.lossycounting.errorrate"; | |||
| public static final String NOT_IMPLEMENTED = "Not implemented"; | |||
|
|
|||
| // Default Snapshot TTL - 30 days (good enough?) | |||
| public static final long DEFAULT_SNAPSHOT_TTL = 24 * 3600 * 30; | |||
There was a problem hiding this comment.
So, what's the usage of the default TTL? If user doesn't specify TTL, should we just use default Snapshot API to take snapshot and keep it forever? If user specifies TTL, we use the value user set.
There was a problem hiding this comment.
If the user doesn't specify TTL, we can take above value as TTL by default. Otherwise we can take user provided TTL anyways. However, if the user decides to keep the snapshot forever(worst case), the user can specify negative value as TTL. e.g.: https://github.com/apache/hbase/pull/371/files#diff-f7c3e813539ba771a69ad518ef8dee00R2772
Does this look good to you?
There was a problem hiding this comment.
We should keep snapshot feature behaving the same as older version, especially if we are going to back-port branch-1 and branch-2, etc., i would suggest keeping snapshot forever by default.
There was a problem hiding this comment.
@Reidddddd Thanks for your review. Should we keep all previous snapshots forever? I thought may be 30 days could be good? IMO if the snapshot is kept for more than a month, it could be a real miss. What is your opinion?
Moreover, I tried to provide one config hbase.master.cleaner.snapshot.disable, which if provided value "true", will prevent any snapshot from getting deleted - especially useful if some backup restore activity is going on for a couple of days.
cc: @apurtell
There was a problem hiding this comment.
If hbase.master.cleaner.snapshot.disable is false by default, then 30 days is ok, since it is explicitly set true.
There was a problem hiding this comment.
Updated the PR. Please review
There was a problem hiding this comment.
What is the benefit of forcing TTL if a user does not specify it explicitly?
Even at other areas where TTL can be used (e.g. Cell TTL) we do not force a default TTL value. In case that is not specified by the user HBase will keep those cells forever.
To keep backward compatibility (even if it is hidden behind a secondary configuration parameter) and consistency with other, similar features I'd prefer to have TTL=FOREVER as default value.
There was a problem hiding this comment.
Sure @petersomogyi I agree with having default TTL as Forever.
Would you be comfortable with this change: https://github.com/apache/hbase/pull/371/files#diff-f7c3e813539ba771a69ad518ef8dee00R2767 ?
There was a problem hiding this comment.
I definitely prefer this version rather than fixed 30 days TTL.
What do you think about having a config parameter e.g. hbase.master.snapshot.ttl.default with value 0 or -1 which means TTL FOREVER. Users can change it 30 days if that is the preferred value for their use case. With this we keep compatibility and users could specify default snapshot TTL based on their needs.
There was a problem hiding this comment.
The default TTL must be "forever". Anything else violates the principle of least surprise.
Use 0 to denote "forever"
| @@ -1864,4 +1864,14 @@ possible configurations would overwhelm and obscure the important. | |||
| <description>Default is 5 minutes. Make it 30 seconds for tests. See | |||
| HBASE-19794 for some context.</description> | |||
| </property> | |||
| <property> | |||
| <name>hbase.master.cleaner.snapshot.interval</name> | |||
| <value>600000</value> | |||
There was a problem hiding this comment.
nit:
Not a strong opinion, I think 10m is too frequent for snapshot cleanup chore.
There was a problem hiding this comment.
Should we keep 30 min or 1 hr?
| private static final Logger LOG = LoggerFactory.getLogger(SnapshotCleanerChore.class); | ||
| private static final String SNAPSHOT_CLEANER_CHORE_NAME = "SnapshotCleaner"; | ||
| private static final String SNAPSHOT_CLEANER_INTERVAL = "hbase.master.cleaner.snapshot.interval"; | ||
| private static final int SNAPSHOT_CLEANER_DEFAULT_INTERVAL = 600 * 1000; // Default 10 min |
There was a problem hiding this comment.
So, you have this 10m defined in both here and the default config file. Is this a duplication?
There was a problem hiding this comment.
Although 10m is defined as default config for hbase.master.cleaner.snapshot.interval, even in the absence of this config, we need to have a default value, which is what is represented by this constant.
Used on L56:
configuration.getInt(SNAPSHOT_CLEANER_INTERVAL, SNAPSHOT_CLEANER_DEFAULT_INTERVAL)
Please let me know if this looks good to you
| final boolean isSnapshotChoreDisabled = this.configuration.getBoolean( | ||
| SNAPSHOT_CLEANER_DISABLE, false); | ||
| if (isSnapshotChoreDisabled) { | ||
| return; |
| @@ -124,6 +124,9 @@ public CompletedSnaphotDirectoriesFilter(FileSystem fs) { | |||
| /** Default value if no start time is specified */ | |||
| public static final long NO_SNAPSHOT_START_TIME_SPECIFIED = 0; | |||
|
|
|||
| // Default value if no ttl is specified for Snapshot | |||
| private static final long NO_SNAPSHOT_TTL_SPECIFIED = 0; | |||
|
|
|||
| public static final String MASTER_SNAPSHOT_TIMEOUT_MILLIS = "hbase.snapshot.master.timeout.millis"; | |||
There was a problem hiding this comment.
Actually, here we use ms for snapshot related timeout, is that a good idea to use second for TTL for consistent concern?
There was a problem hiding this comment.
Good point. However, from the end user perspective, do you think it would be better to provide sec while creating snapshot?
e.g. which one would be preferred for 1 min TTL?
snapshot 't1', 'sp1', {TTL => 60}
snapshot 't1', 'sp1', {TTL => 60000}
multiply by 1000 increases the no for end user to provide with increased TTL
There was a problem hiding this comment.
So far, from shell perspective, it is the former. But i'm not sure if the hbase shell convert the 60s to 60000ms.
There was a problem hiding this comment.
shell doesn't convert it to ms. this will continue to flow as sec only untill this point: https://github.com/apache/hbase/pull/371/files#diff-5fc046382ccaa45c26b89ba3462e8175R92
Please let me know if this looks good to you
There was a problem hiding this comment.
IIRC, hbase shell accepts seconds like cf's TTL, i prefer snapshot 't1', 'sp1', {TTL => 60}. But server side shall be ms, so where the convert happens? Can we follow that convert impl?
|
|
||
| Default Snapshot TTL = 30 Days | ||
|
|
||
| While creating a Snapshot, if TTL in seconds is not specified, the Default `TTL` will be applicable. |
There was a problem hiding this comment.
I don't think using this TTL version snapshot to replace current one is a good idea.
Open to discussion about this more.
There was a problem hiding this comment.
Sorry, I didn't get you. Are you suggesting that we should not keep 30 days as default snapshot TTL?
I am not sure what attribute you are referring to with: replace current one
|
💔 -1 overall
This message was automatically generated. |
| + ((table != null) ? table : null) + " /owner = " + ((owner != null) ? owner : null) | ||
| + (creationTime != -1 ? ("/creationtime = " + creationTime) : "") | ||
| + (version != -1 ? ("/version = " + version) : ""); | ||
| return "SnapshotDescription: name = " + name + "/table = " |
There was a problem hiding this comment.
Can we use StringBuilder to improve the readability?
| @@ -2932,12 +2932,17 @@ public static SnapshotType createSnapshotType(SnapshotProtos.SnapshotDescription | |||
| if (snapshotDesc.getCreationTime() != -1L) { | |||
| builder.setCreationTime(snapshotDesc.getCreationTime()); | |||
| } | |||
| if (snapshotDesc.getTtl() != -1L && snapshotDesc.getTtl() < Long.MAX_VALUE / 1000) { | |||
There was a problem hiding this comment.
snapshotDesc.getTtl() < Long.MAX_VALUE / 1000, not quite understand here, would you mind explaining it?
There was a problem hiding this comment.
Hmm, you are converting the time to ms, i get you.
There was a problem hiding this comment.
TimeUnit has utility functions for this, better than dividing by constants as the intent is clear.
| @@ -35,15 +35,16 @@ message SnapshotDescription { | |||
| required string name = 1; | |||
| optional string table = 2; // not needed for delete, but checked for in taking snapshot | |||
| optional int64 creation_time = 3 [default = 0]; | |||
| optional int64 ttl = 4 [default = 0]; | |||
There was a problem hiding this comment.
It will break wire compatibility, ttl should be appended at the end of parameters sequence.
There was a problem hiding this comment.
Makes sense. Will change this
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-22648-master
branch
from
16492cb
to
f734cd1
Compare
| @@ -2932,12 +2932,17 @@ public static SnapshotType createSnapshotType(SnapshotProtos.SnapshotDescription | |||
| if (snapshotDesc.getCreationTime() != -1L) { | |||
| builder.setCreationTime(snapshotDesc.getCreationTime()); | |||
| } | |||
| if (snapshotDesc.getTtl() != -1L && snapshotDesc.getTtl() < Long.MAX_VALUE / 1000) { | |||
There was a problem hiding this comment.
TimeUnit has utility functions for this, better than dividing by constants as the intent is clear.
| @@ -1445,6 +1445,9 @@ | |||
| "hbase.util.default.lossycounting.errorrate"; | |||
| public static final String NOT_IMPLEMENTED = "Not implemented"; | |||
|
|
|||
| // Default Snapshot TTL - 30 days (good enough?) | |||
| public static final long DEFAULT_SNAPSHOT_TTL = 24 * 3600 * 30; | |||
There was a problem hiding this comment.
The default TTL must be "forever". Anything else violates the principle of least surprise.
Use 0 to denote "forever"
| <value>false</value> | ||
| <description> | ||
| If Snapshot is created without specifying TTL, we can choose to | ||
| apply default TTL(30 days). If this config value is set to true, |
There was a problem hiding this comment.
Comment does not make sense when default TTL is "forever". See above. Default TTL must be forever.
| @Override | ||
| protected void chore() { | ||
| final boolean isSnapshotChoreDisabled = this.configuration.getBoolean( | ||
| SNAPSHOT_CLEANER_DISABLE, false); |
There was a problem hiding this comment.
If disabled do not create the chore.
There is nothing useful about a chore that wakes up only to log repeatedly that it will do nothing. :-)
| LOG.debug("Snapshot Cleaner Chore is disabled. Not performing any cleanup..."); | ||
| return; | ||
| } | ||
| LOG.trace("Snapshot Cleaner Chore is starting up..."); |
There was a problem hiding this comment.
All LOG.debug and LOG.trace invocations should be guarded by if (LOG.isDebugEnabled()) or if (LOG.isTraceEnabled())
| long snapshotTtl = snapshotDescription.getTtl(); | ||
| /* | ||
| * Backward compatibility after the patch deployment on HMaster | ||
| * Any snapshot with negative or zero ttl should not be deleted |
There was a problem hiding this comment.
Again call 0 ttl "forever" and then backwards compat is trivial. This comment should also be updated to note that 0 TTL is forever.
| * Default ttl value specified by {@HConstants.DEFAULT_SNAPSHOT_TTL} | ||
| */ | ||
| if (snapshotCreatedTime > 0 && snapshotTtl > 0 && | ||
| snapshotTtl < (Long.MAX_VALUE / 1000)) { |
There was a problem hiding this comment.
Use TimeUnit for time unit conversions
| @@ -99,6 +100,7 @@ | |||
| <% } %> | |||
| </td> | |||
| <td><%= new Date(snapshot.getCreationTime()) %></td> | |||
| <td><%= snapshot.getTtl() %></td> | |||
There was a problem hiding this comment.
Can print "FOREVER" here or something similar if value is 0, so the user is less likely to be confused
|
💔 -1 overall
This message was automatically generated. |
| * @throws SnapshotCreationException if snapshot creation failed | ||
| * @throws IllegalArgumentException if the snapshot request is formatted incorrectly | ||
| */ | ||
| default void snapshot(String snapshotName, TableName tableName, SnapshotType type, Long ttl) |
There was a problem hiding this comment.
TTL may be the first of more than one snapshot attribute we'd want to pass through this API. We can create a new method (and more backwards compat methods) every time, or make only one change: instead of parameter 'ttl' of type Long, introduce a parameter 'attributes' or 'properties' of type Map<String,Object> or java.util.Properties?
| } | ||
|
|
||
| public SnapshotDescription(String name, TableName table, SnapshotType type, String owner, | ||
| long creationTime, int version) { | ||
| long creationTime, long ttl, int version) { |
There was a problem hiding this comment.
Likewise I would say that the current set of method parameters here are fine but 'ttl' is more of a minor or situational attribute of the snapshot. We should stop creating more SnapshotDescription constructors and introduce a generic map for minor attributes. For your consideration.
There was a problem hiding this comment.
Totally Agree with having minor attributes Map. Also, given that you are fine with having getters and setters of ttl present for SnapshotDescription and protos have well usable builder pattern, I believe it's fine to keep adding each minor attribute explicitly to protos: Snapshot.proto and HBase.proto
| @@ -139,15 +146,27 @@ public long getCreationTime() { | |||
| return this.creationTime; | |||
| } | |||
|
|
|||
| // get snapshot ttl in sec | |||
| public long getTtl() { | |||
There was a problem hiding this comment.
Getters and setters for ttl is fine even if also using generic attribute map with getters and setters for elements of same.
| public static final long DEFAULT_SNAPSHOT_TTL = 0; | ||
|
|
||
| // User defined Default TTL config key | ||
| public static final String DEFAULT_SNAPSHOT_TTL_CONFIG_KEY = "hbase.master.snapshot.ttl.default"; |
There was a problem hiding this comment.
This should be "hbase.master.snapshot.ttl" because you are setting an effective value.
(Calling the default constant DEFAULT_SNAPSHOT_TTL is fine because that actually is naming a default value. )
Minor point. If you like.
| </description> | ||
| </property> | ||
| <property> | ||
| <name>hbase.master.snapshot.ttl.default</name> |
|
LGTM, and I think Andrew's comments are good and should be followed. |
|
💔 -1 overall
This message was automatically generated. |
Thanks @Reidddddd |
virajjasani
force-pushed
the
HBASE-22648-master
branch
from
b745f8a
to
7ac6fc9
Compare
|
Not sure why the build is not auto-triggered. Can someone please help me retrigger the build: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-371/ Updated the patch based on latest review comments. Please review @apurtell |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
force-pushed
the
HBASE-22648-master
branch
from
fb7b3a7
to
a2b1895
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The PR is approved. Will merge/commit and pick back these changes. Thanks @virajjasani |
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Co-authored-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/SnapshotDescription.java hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java hbase-common/src/main/resources/hbase-default.xml hbase-protocol-shaded/src/main/protobuf/Snapshot.proto hbase-protocol/src/main/protobuf/HBase.proto hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotInfo.java hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp hbase-server/src/main/resources/hbase-webapps/master/snapshotsStats.jsp hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotFromClient.java hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotTemporaryDirectory.java hbase-shell/src/main/ruby/hbase/admin.rb src/main/asciidoc/_chapters/hbase-default.adoc
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
