[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r304135678
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -1372,6 +1372,51 @@ default void snapshot(String snapshotName, TableName
tableName, SnapshotType typ
snapshot(new SnapshotDescription(snapshotName, tableName, type));
}
+ /**
+ * Create typed snapshot of the table. Snapshots are considered unique based
on the name of the
+ * snapshot. Snapshots are taken sequentially even when requested
concurrently, across
+ * all tables. Attempts to take a snapshot with the same name (even a
different type or with
+ * different parameters) will fail with a {@link SnapshotCreationException}
indicating the
+ * duplicate naming. Snapshot names follow the same naming constraints as
tables in HBase. See
+ * {@link
org.apache.hadoop.hbase.TableName#isLegalFullyQualifiedTableName(byte[])}.
+ * Snapshot can live with ttl seconds.
+ *
+ * @param snapshotName name to give the snapshot on the filesystem. Must be
unique from all other
+ * snapshots stored on the cluster
+ * @param tableNamename of the table to snapshot
+ * @param type type of snapshot to take
+ * @param ttl time to live in seconds. After expiry, the snapshot
should be deleted
+ * @throws IOException we fail to reach the master
+ * @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)
Review 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 or java.util.Properties?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r304136426
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/SnapshotDescription.java
##
@@ -139,15 +146,27 @@ public long getCreationTime() {
return this.creationTime;
}
+ // get snapshot ttl in sec
+ public long getTtl() {
Review comment:
Getters and setters for ttl is fine even if also using generic attribute map
with getters and setters for elements of same.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL URL: https://github.com/apache/hbase/pull/371#discussion_r304136760 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -1445,6 +1445,15 @@ "hbase.util.default.lossycounting.errorrate"; public static final String NOT_IMPLEMENTED = "Not implemented"; + // Default TTL - FOREVER + 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"; Review 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. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r304136202
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/SnapshotDescription.java
##
@@ -63,46 +64,52 @@ public SnapshotDescription(String name, String table,
SnapshotType type) {
}
public SnapshotDescription(String name, TableName table, SnapshotType type) {
-this(name, table, type, null);
+this(name, table, type, null, -1, -1, -1);
}
/**
- * @deprecated since 2.0.0 and will be removed in 3.0.0. Use the version
with the TableName
- * instance instead.
* @see #SnapshotDescription(String, TableName, SnapshotType, String)
* @see https://issues.apache.org/jira/browse/HBASE-16892;>HBASE-16892
+ * @deprecated since 2.0.0 and will be removed in 3.0.0. Use the version
with the TableName
+ * instance instead.
*/
@Deprecated
public SnapshotDescription(String name, String table, SnapshotType type,
String owner) {
this(name, TableName.valueOf(table), type, owner);
}
public SnapshotDescription(String name, TableName table, SnapshotType type,
String owner) {
-this(name, table, type, owner, -1, -1);
+this(name, table, type, owner, -1, -1, -1);
}
/**
+ * @see #SnapshotDescription(String, TableName, SnapshotType, String, long,
long, int)
+ * @see https://issues.apache.org/jira/browse/HBASE-16892;>HBASE-16892
* @deprecated since 2.0.0 and will be removed in 3.0.0. Use the version
with the TableName
* instance instead.
- * @see #SnapshotDescription(String, TableName, SnapshotType, String, long,
int)
- * @see https://issues.apache.org/jira/browse/HBASE-16892;>HBASE-16892
*/
@Deprecated
public SnapshotDescription(String name, String table, SnapshotType type,
String owner,
long creationTime, int version) {
-this(name, TableName.valueOf(table), type, owner, creationTime, version);
+this(name, TableName.valueOf(table), type, owner, creationTime, -1,
version);
}
public SnapshotDescription(String name, TableName table, SnapshotType type,
String owner,
- long creationTime, int version) {
+ long creationTime, long ttl, int version) {
Review 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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL URL: https://github.com/apache/hbase/pull/371#discussion_r304137323 ## File path: hbase-common/src/main/resources/hbase-default.xml ## @@ -1864,4 +1864,23 @@ possible configurations would overwhelm and obscure the important. Default is 5 minutes. Make it 30 seconds for tests. See HBASE-19794 for some context. + +hbase.master.cleaner.snapshot.interval +180 + + Snapshot Cleanup chore interval in milliseconds. + The cleanup thread keeps running at this interval + to find all snapshots that are expired based on TTL + and delete them. + + + +hbase.master.snapshot.ttl.default Review comment: Same This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303592367
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -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) {
Review comment:
TimeUnit has utility functions for this, better than dividing by constants
as the intent is clear.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL URL: https://github.com/apache/hbase/pull/371#discussion_r303592755 ## File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java ## @@ -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; Review comment: The default TTL must be "forever". Anything else violates the principle of least surprise. Use 0 to denote "forever" This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303594344
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/SnapshotCleanerChore.java
##
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.master.cleaner;
+
+
+import java.io.IOException;
+import java.util.List;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.ScheduledChore;
+import org.apache.hadoop.hbase.Stoppable;
+import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
+import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
+
+/**
+ * This chore, every time it runs, will try to delete snapshots that are
expired based on TTL in
+ * seconds configured for each Snapshot
+ */
+@InterfaceAudience.Private
+public class SnapshotCleanerChore extends ScheduledChore {
+
+ 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 = 1800 * 1000; //
Default 30 min
+ private static final String SNAPSHOT_CLEANER_DISABLE =
"hbase.master.cleaner.snapshot.disable";
+ private static final String DELETE_SNAPSHOT_EVENT =
+ "Eligible Snapshot for cleanup due to expired TTL.";
+
+ private final SnapshotManager snapshotManager;
+ private final Configuration configuration;
+
+ /**
+ * Construct Snapshot Cleaner Chore with parameterized constructor
+ *
+ * @param stopper When {@link Stoppable#isStopped()} is true, this chore
will cancel and cleanup
+ * @param configuration The configuration to set
+ * @param snapshotManager SnapshotManager instance to manage lifecycle of
snapshot
+ */
+ public SnapshotCleanerChore(Stoppable stopper, Configuration configuration,
+ SnapshotManager snapshotManager) {
+super(SNAPSHOT_CLEANER_CHORE_NAME, stopper,
configuration.getInt(SNAPSHOT_CLEANER_INTERVAL,
+SNAPSHOT_CLEANER_DEFAULT_INTERVAL));
+this.snapshotManager = snapshotManager;
+this.configuration = configuration;
+ }
+
+ @Override
+ protected void chore() {
+final boolean isSnapshotChoreDisabled = this.configuration.getBoolean(
+SNAPSHOT_CLEANER_DISABLE, false);
+if (isSnapshotChoreDisabled) {
+ LOG.debug("Snapshot Cleaner Chore is disabled. Not performing any
cleanup...");
+ return;
+}
+LOG.trace("Snapshot Cleaner Chore is starting up...");
Review comment:
All LOG.debug and LOG.trace invocations should be guarded by if
(LOG.isDebugEnabled()) or if (LOG.isTraceEnabled())
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303594642
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/SnapshotCleanerChore.java
##
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.master.cleaner;
+
+
+import java.io.IOException;
+import java.util.List;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.ScheduledChore;
+import org.apache.hadoop.hbase.Stoppable;
+import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
+import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
+
+/**
+ * This chore, every time it runs, will try to delete snapshots that are
expired based on TTL in
+ * seconds configured for each Snapshot
+ */
+@InterfaceAudience.Private
+public class SnapshotCleanerChore extends ScheduledChore {
+
+ 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 = 1800 * 1000; //
Default 30 min
+ private static final String SNAPSHOT_CLEANER_DISABLE =
"hbase.master.cleaner.snapshot.disable";
+ private static final String DELETE_SNAPSHOT_EVENT =
+ "Eligible Snapshot for cleanup due to expired TTL.";
+
+ private final SnapshotManager snapshotManager;
+ private final Configuration configuration;
+
+ /**
+ * Construct Snapshot Cleaner Chore with parameterized constructor
+ *
+ * @param stopper When {@link Stoppable#isStopped()} is true, this chore
will cancel and cleanup
+ * @param configuration The configuration to set
+ * @param snapshotManager SnapshotManager instance to manage lifecycle of
snapshot
+ */
+ public SnapshotCleanerChore(Stoppable stopper, Configuration configuration,
+ SnapshotManager snapshotManager) {
+super(SNAPSHOT_CLEANER_CHORE_NAME, stopper,
configuration.getInt(SNAPSHOT_CLEANER_INTERVAL,
+SNAPSHOT_CLEANER_DEFAULT_INTERVAL));
+this.snapshotManager = snapshotManager;
+this.configuration = configuration;
+ }
+
+ @Override
+ protected void chore() {
+final boolean isSnapshotChoreDisabled = this.configuration.getBoolean(
+SNAPSHOT_CLEANER_DISABLE, false);
+if (isSnapshotChoreDisabled) {
+ LOG.debug("Snapshot Cleaner Chore is disabled. Not performing any
cleanup...");
+ return;
+}
+LOG.trace("Snapshot Cleaner Chore is starting up...");
+try {
+ List completedSnapshotsList =
+ this.snapshotManager.getCompletedSnapshots();
+ for (SnapshotProtos.SnapshotDescription snapshotDescription :
completedSnapshotsList) {
+long snapshotCreatedTime = snapshotDescription.getCreationTime();
+long snapshotTtl = snapshotDescription.getTtl();
+/*
+ * Backward compatibility after the patch deployment on HMaster
+ * Any snapshot with negative or zero ttl should not be deleted
+ * Default ttl value specified by {@HConstants.DEFAULT_SNAPSHOT_TTL}
+ */
+if (snapshotCreatedTime > 0 && snapshotTtl > 0 &&
+snapshotTtl < (Long.MAX_VALUE / 1000)) {
Review comment:
Use TimeUnit for time unit conversions
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303593588
##
File path: hbase-protocol-shaded/src/main/protobuf/Snapshot.proto
##
@@ -44,6 +44,7 @@ message SnapshotDescription {
optional int32 version = 5;
optional string owner = 6;
optional UsersAndPermissions users_and_permissions = 7;
+ optional int64 ttl = 8 [default = 0];
Review comment:
Here default value is 0. So I guess this answers the question of what should
denote "forever" :-)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL URL: https://github.com/apache/hbase/pull/371#discussion_r303595006 ## File path: hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp ## @@ -99,6 +100,7 @@ <% } %> <%= new Date(snapshot.getCreationTime()) %> +<%= snapshot.getTtl() %> Review comment: Can print "FOREVER" here or something similar if value is 0, so the user is less likely to be confused This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303594585
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/SnapshotCleanerChore.java
##
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.master.cleaner;
+
+
+import java.io.IOException;
+import java.util.List;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.ScheduledChore;
+import org.apache.hadoop.hbase.Stoppable;
+import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
+import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
+
+/**
+ * This chore, every time it runs, will try to delete snapshots that are
expired based on TTL in
+ * seconds configured for each Snapshot
+ */
+@InterfaceAudience.Private
+public class SnapshotCleanerChore extends ScheduledChore {
+
+ 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 = 1800 * 1000; //
Default 30 min
+ private static final String SNAPSHOT_CLEANER_DISABLE =
"hbase.master.cleaner.snapshot.disable";
+ private static final String DELETE_SNAPSHOT_EVENT =
+ "Eligible Snapshot for cleanup due to expired TTL.";
+
+ private final SnapshotManager snapshotManager;
+ private final Configuration configuration;
+
+ /**
+ * Construct Snapshot Cleaner Chore with parameterized constructor
+ *
+ * @param stopper When {@link Stoppable#isStopped()} is true, this chore
will cancel and cleanup
+ * @param configuration The configuration to set
+ * @param snapshotManager SnapshotManager instance to manage lifecycle of
snapshot
+ */
+ public SnapshotCleanerChore(Stoppable stopper, Configuration configuration,
+ SnapshotManager snapshotManager) {
+super(SNAPSHOT_CLEANER_CHORE_NAME, stopper,
configuration.getInt(SNAPSHOT_CLEANER_INTERVAL,
+SNAPSHOT_CLEANER_DEFAULT_INTERVAL));
+this.snapshotManager = snapshotManager;
+this.configuration = configuration;
+ }
+
+ @Override
+ protected void chore() {
+final boolean isSnapshotChoreDisabled = this.configuration.getBoolean(
+SNAPSHOT_CLEANER_DISABLE, false);
+if (isSnapshotChoreDisabled) {
+ LOG.debug("Snapshot Cleaner Chore is disabled. Not performing any
cleanup...");
+ return;
+}
+LOG.trace("Snapshot Cleaner Chore is starting up...");
+try {
+ List completedSnapshotsList =
+ this.snapshotManager.getCompletedSnapshots();
+ for (SnapshotProtos.SnapshotDescription snapshotDescription :
completedSnapshotsList) {
+long snapshotCreatedTime = snapshotDescription.getCreationTime();
+long snapshotTtl = snapshotDescription.getTtl();
+/*
+ * Backward compatibility after the patch deployment on HMaster
+ * Any snapshot with negative or zero ttl should not be deleted
Review 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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303594142
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/SnapshotCleanerChore.java
##
@@ -0,0 +1,114 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.master.cleaner;
+
+
+import java.io.IOException;
+import java.util.List;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.ScheduledChore;
+import org.apache.hadoop.hbase.Stoppable;
+import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
+import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
+
+/**
+ * This chore, every time it runs, will try to delete snapshots that are
expired based on TTL in
+ * seconds configured for each Snapshot
+ */
+@InterfaceAudience.Private
+public class SnapshotCleanerChore extends ScheduledChore {
+
+ 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 = 1800 * 1000; //
Default 30 min
+ private static final String SNAPSHOT_CLEANER_DISABLE =
"hbase.master.cleaner.snapshot.disable";
+ private static final String DELETE_SNAPSHOT_EVENT =
+ "Eligible Snapshot for cleanup due to expired TTL.";
+
+ private final SnapshotManager snapshotManager;
+ private final Configuration configuration;
+
+ /**
+ * Construct Snapshot Cleaner Chore with parameterized constructor
+ *
+ * @param stopper When {@link Stoppable#isStopped()} is true, this chore
will cancel and cleanup
+ * @param configuration The configuration to set
+ * @param snapshotManager SnapshotManager instance to manage lifecycle of
snapshot
+ */
+ public SnapshotCleanerChore(Stoppable stopper, Configuration configuration,
+ SnapshotManager snapshotManager) {
+super(SNAPSHOT_CLEANER_CHORE_NAME, stopper,
configuration.getInt(SNAPSHOT_CLEANER_INTERVAL,
+SNAPSHOT_CLEANER_DEFAULT_INTERVAL));
+this.snapshotManager = snapshotManager;
+this.configuration = configuration;
+ }
+
+ @Override
+ protected void chore() {
+final boolean isSnapshotChoreDisabled = this.configuration.getBoolean(
+SNAPSHOT_CLEANER_DISABLE, false);
Review 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. :-)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL URL: https://github.com/apache/hbase/pull/371#discussion_r303593397 ## File path: hbase-common/src/main/resources/hbase-default.xml ## @@ -1864,4 +1864,25 @@ possible configurations would overwhelm and obscure the important. Default is 5 minutes. Make it 30 seconds for tests. See HBASE-19794 for some context. + +hbase.master.cleaner.snapshot.interval +180 + + Snapshot Cleanup chore interval in milliseconds. + The cleanup thread keeps running at this interval + to find all snapshots that are expired based on TTL + and delete them. + + + +hbase.master.snapshot.apply.default.cleaner.ttl +false + + If Snapshot is created without specifying TTL, we can choose to + apply default TTL(30 days). If this config value is set to true, Review comment: Comment does not make sense when default TTL is "forever". See above. Default TTL must be forever. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #371: HBASE-22648 : Introducing Snapshot TTL
apurtell commented on a change in pull request #371: HBASE-22648 : Introducing
Snapshot TTL
URL: https://github.com/apache/hbase/pull/371#discussion_r303593699
##
File path: hbase-protocol/src/main/protobuf/HBase.proto
##
@@ -184,6 +184,7 @@ message SnapshotDescription {
optional Type type = 4 [default = FLUSH];
optional int32 version = 5;
optional string owner = 6;
+ optional int64 ttl = 7 [default = 0];
Review comment:
And here the default is 0 too
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
