Skip to content

Commit 96fd5eb

Browse files
author
Rushabh
committed
[HBASE-22874] Adding review comments.
1 parent a422417 commit 96fd5eb

File tree

4 files changed

+35
-40
lines changed

4 files changed

+35
-40
lines changed

‎hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ org.apache.hadoop.hbase.quotas.QuotaUtil;
5555
org.apache.hadoop.hbase.security.access.PermissionStorage;
5656
org.apache.hadoop.hbase.security.visibility.VisibilityConstants;
5757
org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos.SnapshotDescription;
58-
org.apache.hadoop.hbase.tool.Canary;
58+
org.apache.hadoop.hbase.tool.CanaryTool;
5959
org.apache.hadoop.hbase.util.Bytes;
6060
org.apache.hadoop.hbase.util.FSUtils;
6161
org.apache.hadoop.hbase.util.JvmVersion;
@@ -513,7 +513,7 @@ AssignmentManager assignmentManager = master.getAssignmentManager();
513513
<%java>String description = null;
514514
if (tableName.equals(TableName.META_TABLE_NAME)){
515515
description = "The hbase:meta table holds references to all User Table regions.";
516-
} else if (tableName.equals(Canary.DEFAULT_WRITE_TABLE_NAME)){
516+
} else if (tableName.equals(CanaryTool.DEFAULT_WRITE_TABLE_NAME)){
517517
description = "The hbase:canary table is used to sniff the write availbility of"
518518
+ " each regionserver.";
519519
} else if (tableName.equals(PermissionStorage.ACL_TABLE_NAME)){

‎hbase-server/src/main/java/org/apache/hadoop/hbase/tool/Canary.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,15 @@
2727
import org.apache.yetus.audience.InterfaceAudience;
2828

2929
@InterfaceAudience.Public
30-
public interface CanaryInterface {
30+
public interface Canary {
3131

32-
static CanaryInterface create(Configuration conf, ExecutorService executor) {
33-
return new Canary(conf, executor);
32+
static Canary create(Configuration conf, ExecutorService executor) {
33+
return new CanaryTool(conf, executor);
3434
}
3535

3636
@VisibleForTesting
37-
static CanaryInterface create(Configuration conf, ExecutorService executor, Canary.Sink sink) {
38-
return new Canary(conf, executor, sink);
37+
static Canary create(Configuration conf, ExecutorService executor, CanaryTool.Sink sink) {
38+
return new CanaryTool(conf, executor, sink);
3939
}
4040

4141
/**

‎hbase-server/src/main/java/org/apache/hadoop/hbase/tool/CanaryTool.java‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@
8080
import org.apache.hadoop.hbase.client.Table;
8181
import org.apache.hadoop.hbase.client.TableDescriptor;
8282
import org.apache.hadoop.hbase.filter.FirstKeyOnlyFilter;
83-
import org.apache.hadoop.hbase.tool.Canary.RegionTask.TaskType;
83+
import org.apache.hadoop.hbase.tool.CanaryTool.RegionTask.TaskType;
8484
import org.apache.hadoop.hbase.util.Bytes;
8585
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
8686
import org.apache.hadoop.hbase.util.Pair;
@@ -120,7 +120,7 @@
120120
* </ol>
121121
*/
122122
@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.TOOLS)
123-
public class Canary implements Tool, CanaryInterface {
123+
public class CanaryTool implements Tool, Canary {
124124

125125
@Override
126126
public int checkRegions(String[] targets) throws Exception {
@@ -610,6 +610,7 @@ public Void call() {
610610
private static final String CANARY_TABLE_FAMILY_NAME = "Test";
611611

612612
private Configuration conf = null;
613+
private long interval = 0;
613614
private Sink sink = null;
614615

615616
/**
@@ -644,35 +645,32 @@ public Void call() {
644645
public static final String HBASE_CANARY_ZOOKEEPER_PERMITTED_FAILURES
645646
= "hbase.canary.zookeeper.permitted.failures";
646647

647-
public static final String HBASE_CANARY_INTERVAL = "hbase.canary.interval";
648-
public static final String HBASE_CANARY_TREAT_FAILURE_AS_ERROR
649-
= "hbase.canary.treat.failure.as.error";
650648
public static final String HBASE_CANARY_USE_REGEX = "hbase.canary.use.regex";
651649
public static final String HBASE_CANARY_TIMEOUT = "hbase.canary.timeout";
652650
public static final String HBASE_CANARY_FAIL_ON_ERROR = "hbase.canary.fail.on.error";
653651

654652

655653
private ExecutorService executor; // threads to retrieve data from regionservers
656654

657-
public Canary() {
655+
public CanaryTool() {
658656
this(new ScheduledThreadPoolExecutor(1));
659657
}
660658

661-
public Canary(ExecutorService executor) {
659+
public CanaryTool(ExecutorService executor) {
662660
this(executor, null);
663661
}
664662

665663
@VisibleForTesting
666-
Canary(ExecutorService executor, Sink sink) {
664+
CanaryTool(ExecutorService executor, Sink sink) {
667665
this.executor = executor;
668666
this.sink = sink;
669667
}
670668

671-
Canary(Configuration conf, ExecutorService executor) {
669+
CanaryTool(Configuration conf, ExecutorService executor) {
672670
this(conf, executor, null);
673671
}
674672

675-
Canary(Configuration conf, ExecutorService executor, Sink sink) {
673+
CanaryTool(Configuration conf, ExecutorService executor, Sink sink) {
676674
this(executor, sink);
677675
setConf(conf);
678676
}
@@ -692,7 +690,7 @@ public void setConf(Configuration conf) {
692690

693691
private int parseArgs(String[] args) {
694692
int index = -1;
695-
long interval = 0, permittedFailures = 0;
693+
long permittedFailures = 0;
696694
boolean regionServerAllRegions = false, writeSniffing = false;
697695
String readTableTimeoutsStr = null;
698696

@@ -712,8 +710,7 @@ private int parseArgs(String[] args) {
712710
printUsageAndExit();
713711
} else if (cmd.equals("-daemon") && interval == 0) {
714712
// user asked for daemon mode, set a default interval between checks
715-
conf.setLong(HBASE_CANARY_INTERVAL, DEFAULT_INTERVAL);
716-
713+
interval = DEFAULT_INTERVAL;
717714
} else if (cmd.equals("-interval")) {
718715
// user has specified an interval for canary breaths (-interval N)
719716
i++;
@@ -729,7 +726,6 @@ private int parseArgs(String[] args) {
729726
System.err.println("-interval needs a numeric value argument.");
730727
printUsageAndExit();
731728
}
732-
conf.setLong(HBASE_CANARY_INTERVAL, interval);
733729
} else if (cmd.equals("-zookeeper")) {
734730
this.zookeeperMode = true;
735731
} else if(cmd.equals("-regionserver")) {
@@ -741,7 +737,7 @@ private int parseArgs(String[] args) {
741737
writeSniffing = true;
742738
conf.setBoolean(HBASE_CANARY_REGION_WRITE_SNIFFING, true);
743739
} else if(cmd.equals("-treatFailureAsError") || cmd.equals("-failureAsError")) {
744-
conf.setBoolean(HBASE_CANARY_TREAT_FAILURE_AS_ERROR, true);
740+
conf.setBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
745741
} else if (cmd.equals("-e")) {
746742
conf.setBoolean(HBASE_CANARY_USE_REGEX, true);
747743
} else if (cmd.equals("-t")) {
@@ -887,7 +883,6 @@ private int runMonitor(String[] monitorTargets) throws Exception {
887883
long currentTimeLength = 0;
888884
boolean failOnError = conf.getBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
889885
long timeout = conf.getLong(HBASE_CANARY_TIMEOUT, DEFAULT_TIMEOUT);
890-
long interval = conf.getLong(HBASE_CANARY_INTERVAL, 0);
891886
// Get a connection to use in below.
892887
try (Connection connection = ConnectionFactory.createConnection(this.conf)) {
893888
do {
@@ -1010,7 +1005,7 @@ private Monitor newMonitor(final Connection connection, String[] monitorTargets)
10101005
boolean regionServerAllRegions
10111006
= conf.getBoolean(HBASE_CANARY_REGIONSERVER_ALL_REGIONS, false);
10121007
boolean treatFailureAsError
1013-
= conf.getBoolean(HBASE_CANARY_TREAT_FAILURE_AS_ERROR, false);
1008+
= conf.getBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
10141009
int permittedFailures
10151010
= conf.getInt(HBASE_CANARY_ZOOKEEPER_PERMITTED_FAILURES, 0);
10161011
boolean writeSniffing
@@ -1217,7 +1212,7 @@ public void run() {
12171212
this.initialized = true;
12181213
for (String table : tables) {
12191214
LongAdder readLatency = regionSink.initializeAndGetReadLatencyForTable(table);
1220-
taskFutures.addAll(Canary.sniff(admin, regionSink, table, executor, TaskType.READ,
1215+
taskFutures.addAll(CanaryTool.sniff(admin, regionSink, table, executor, TaskType.READ,
12211216
this.rawScanEnabled, readLatency));
12221217
}
12231218
} else {
@@ -1236,7 +1231,7 @@ public void run() {
12361231
// sniff canary table with write operation
12371232
regionSink.initializeWriteLatency();
12381233
LongAdder writeTableLatency = regionSink.getWriteLatency();
1239-
taskFutures.addAll(Canary.sniff(admin, regionSink, admin.getDescriptor(writeTableName),
1234+
taskFutures.addAll(CanaryTool.sniff(admin, regionSink, admin.getDescriptor(writeTableName),
12401235
executor, TaskType.WRITE, this.rawScanEnabled, writeTableLatency));
12411236
}
12421237

@@ -1341,7 +1336,7 @@ private List<Future<Void>> sniff(TaskType taskType, RegionStdOutSink regionSink)
13411336
(!td.getTableName().equals(writeTableName))) {
13421337
LongAdder readLatency =
13431338
regionSink.initializeAndGetReadLatencyForTable(td.getTableName().getNameAsString());
1344-
taskFutures.addAll(Canary.sniff(admin, sink, td, executor, taskType, this.rawScanEnabled,
1339+
taskFutures.addAll(CanaryTool.sniff(admin, sink, td, executor, taskType, this.rawScanEnabled,
13451340
readLatency));
13461341
}
13471342
}
@@ -1414,7 +1409,7 @@ private static List<Future<Void>> sniff(final Admin admin, final Sink sink, Stri
14141409
throws Exception {
14151410
LOG.debug("Checking table is enabled and getting table descriptor for table {}", tableName);
14161411
if (admin.isTableEnabled(TableName.valueOf(tableName))) {
1417-
return Canary.sniff(admin, sink, admin.getDescriptor(TableName.valueOf(tableName)),
1412+
return CanaryTool.sniff(admin, sink, admin.getDescriptor(TableName.valueOf(tableName)),
14181413
executor, taskType, rawScanEnabled, readLatency);
14191414
} else {
14201415
LOG.warn("Table {} is not enabled", tableName);
@@ -1725,7 +1720,7 @@ public static void main(String[] args) throws Exception {
17251720
int exitCode;
17261721
ExecutorService executor = new ScheduledThreadPoolExecutor(numThreads);
17271722
try {
1728-
exitCode = ToolRunner.run(conf, new Canary(executor), args);
1723+
exitCode = ToolRunner.run(conf, new CanaryTool(executor), args);
17291724
} finally {
17301725
executor.shutdown();
17311726
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/tool/TestCanaryTool.java‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ public void testBasicCanaryWorks() throws Exception {
115115
table.put(p);
116116
}
117117
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
118-
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
119-
Canary canary = new Canary(executor, sink);
118+
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
119+
CanaryTool canary = new CanaryTool(executor, sink);
120120
String[] args = { "-writeSniffing", "-t", "10000", tableName.getNameAsString() };
121121
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
122122
assertEquals("verify no read error count", 0, canary.getReadFailures().size());
@@ -142,8 +142,8 @@ public void testReadTableTimeouts() throws Exception {
142142
}
143143
}
144144
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
145-
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
146-
Canary canary = new Canary(executor, sink);
145+
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
146+
CanaryTool canary = new CanaryTool(executor, sink);
147147
String configuredTimeoutStr = tableNames[0].getNameAsString() + "=" + Long.MAX_VALUE + "," +
148148
tableNames[1].getNameAsString() + "=0";
149149
String[] args = {"-readTableTimeouts", configuredTimeoutStr, name.getMethodName() + "1",
@@ -172,8 +172,8 @@ public boolean matches(LoggingEvent argument) {
172172
@Test
173173
public void testWriteTableTimeout() throws Exception {
174174
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
175-
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
176-
Canary canary = new Canary(executor, sink);
175+
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
176+
CanaryTool canary = new CanaryTool(executor, sink);
177177
String[] args = { "-writeSniffing", "-writeTableTimeout", String.valueOf(Long.MAX_VALUE)};
178178
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
179179
assertNotEquals("verify non-null write latency", null, sink.getWriteLatency());
@@ -225,8 +225,8 @@ public void testRawScanConfig() throws Exception {
225225
table.put(p);
226226
}
227227
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
228-
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
229-
Canary canary = new Canary(executor, sink);
228+
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
229+
CanaryTool canary = new CanaryTool(executor, sink);
230230
String[] args = { "-t", "10000", name.getMethodName() };
231231
org.apache.hadoop.conf.Configuration conf =
232232
new org.apache.hadoop.conf.Configuration(testingUtility.getConfiguration());
@@ -240,7 +240,7 @@ public void testRawScanConfig() throws Exception {
240240

241241
private void runRegionserverCanary() throws Exception {
242242
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
243-
Canary canary = new Canary(executor, new Canary.RegionServerStdOutSink());
243+
CanaryTool canary = new CanaryTool(executor, new CanaryTool.RegionServerStdOutSink());
244244
String[] args = { "-t", "10000", "-regionserver"};
245245
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
246246
assertEquals("verify no read error count", 0, canary.getReadFailures().size());
@@ -251,8 +251,8 @@ private void testZookeeperCanaryWithArgs(String[] args) throws Exception {
251251
Iterables.getOnlyElement(testingUtility.getZkCluster().getClientPortList(), null);
252252
testingUtility.getConfiguration().set(HConstants.ZOOKEEPER_QUORUM, "localhost:" + port);
253253
ExecutorService executor = new ScheduledThreadPoolExecutor(2);
254-
Canary.ZookeeperStdOutSink sink = spy(new Canary.ZookeeperStdOutSink());
255-
Canary canary = new Canary(executor, sink);
254+
CanaryTool.ZookeeperStdOutSink sink = spy(new CanaryTool.ZookeeperStdOutSink());
255+
CanaryTool canary = new CanaryTool(executor, sink);
256256
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
257257

258258
String baseZnode = testingUtility.getConfiguration()

0 commit comments

Comments
 (0)