[
https://issues.apache.org/jira/browse/HBASE-22760?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16914543#comment-16914543
]
Andrew Purtell edited comment on HBASE-22760 at 8/23/19 6:49 PM:
-----------------------------------------------------------------
I know this is lame and bikeshedding but I really don't like those command
names. Too many chars, even if we have autocomplete. Drop the "_auto_"? Same
for all of the code changes too. Snapshot cleanup is "auto" by definition.
Otherwise the patch mostly lgtm.
Shame about needing yet another znode, but that is how we have implemented the
other switches, makes sense to do it this way, doesn't make much sense to do it
differently.
Another minor naming issue. Here:
{noformat}
diff --git a/hbase-protocol-shaded/src/main/protobuf/Master.proto
b/hbase-protocol-shaded/src/main/protobuf/Master.proto
index 3429d0343d..2fd0f10177 100644
--- a/hbase-protocol-shaded/src/main/protobuf/Master.proto
+++ b/hbase-protocol-shaded/src/main/protobuf/Master.proto
@@ -318,6 +318,22 @@ enum MasterSwitchType {
MERGE = 1;
}
+message SetSnapshotAutoCleanupRequest {
+ required bool on = 1;
+ optional bool synchronous = 2;
+}
+
+message SetSnapshotAutoCleanupResponse {
+ required bool prev_snapshot_auto_cleanup = 1;
+}
+
+message IsSnapshotAutoCleanupEnabledRequest {
+}
+
+message IsSnapshotAutoCleanupEnabledResponse {
+ required bool enabled = 1;
+}
{noformat}
Why "on" where everywhere else "enabled"?
Another minor thing, I think you can synchronize on the SnapshotCleanerChore
instance itself and not need to export a mutex object.
was (Author: apurtell):
I know this is lame and bikeshedding but I really don't like those command
names. Too many chars, even if we have autocomplete. Drop the "_auto_"? Same
for all of the code changes too. Snapshot cleanup is "auto" by definition.
Otherwise the patch lgtm. Shame about needing yet another znode, but that is
how we have implemented the other switches, makes sense to do it this way,
doesn't make much sense to do it differently.
Another minor naming issue. Here:
{noformat}
diff --git a/hbase-protocol-shaded/src/main/protobuf/Master.proto
b/hbase-protocol-shaded/src/main/protobuf/Master.proto
index 3429d0343d..2fd0f10177 100644
--- a/hbase-protocol-shaded/src/main/protobuf/Master.proto
+++ b/hbase-protocol-shaded/src/main/protobuf/Master.proto
@@ -318,6 +318,22 @@ enum MasterSwitchType {
MERGE = 1;
}
+message SetSnapshotAutoCleanupRequest {
+ required bool on = 1;
+ optional bool synchronous = 2;
+}
+
+message SetSnapshotAutoCleanupResponse {
+ required bool prev_snapshot_auto_cleanup = 1;
+}
+
+message IsSnapshotAutoCleanupEnabledRequest {
+}
+
+message IsSnapshotAutoCleanupEnabledResponse {
+ required bool enabled = 1;
+}
{noformat}
Why "on" where everywhere else "enabled"?
Another minor thing, I think you can synchronize on the SnapshotCleanerChore
instance itself and not need to export a mutex object.
> Stop/Resume Snapshot Auto-Cleanup activity with shell command
> -------------------------------------------------------------
>
> Key: HBASE-22760
> URL: https://issues.apache.org/jira/browse/HBASE-22760
> Project: HBase
> Issue Type: Improvement
> Components: Admin, shell, snapshots
> Affects Versions: 3.0.0, 1.5.0, 2.3.0, 2.2.1, 1.4.11
> Reporter: Viraj Jasani
> Assignee: Viraj Jasani
> Priority: Major
> Fix For: 3.0.0, 1.5.0, 2.3.0, 2.2.1, 1.4.11
>
> Attachments: HBASE-22760.master.003.patch,
> HBASE-22760.master.004.patch, HBASE-22760.master.005.patch
>
>
> For any scheduled snapshot backup activity, we would like to disable
> auto-cleaner for snapshot based on TTL. However, as per HBASE-22648 we have a
> config to disable snapshot auto-cleaner:
> hbase.master.cleaner.snapshot.disable, which would take effect only upon
> HMaster restart just similar to any other hbase-site configs.
> For any running cluster, we should be able to stop/resume auto-cleanup
> activity for snapshot based on shell command. Something similar to below
> command should be able to stop/start cleanup chore:
> hbase(main):001:0> snapshot_auto_cleanup_switch false (disable
> auto-cleaner)
> hbase(main):001:0> snapshot_auto_cleanup_switch true (enable auto-cleaner)
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
