Skip to content

Commit

Permalink
[HBASE-22874] Addressing review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
Rushabh committed Sep 17, 2019
1 parent 2cdcbda commit 80c3bac
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 49 deletions.
4 changes: 2 additions & 2 deletions hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ org.apache.hadoop.hbase.quotas.QuotaUtil;
org.apache.hadoop.hbase.security.access.PermissionStorage;
org.apache.hadoop.hbase.security.visibility.VisibilityConstants;
org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos.SnapshotDescription;
org.apache.hadoop.hbase.tool.Canary;
org.apache.hadoop.hbase.tool.CanaryTool;
org.apache.hadoop.hbase.util.Bytes;
org.apache.hadoop.hbase.util.FSUtils;
org.apache.hadoop.hbase.util.JvmVersion;
Expand Down Expand Up @@ -513,7 +513,7 @@ AssignmentManager assignmentManager = master.getAssignmentManager();
<%java>String description = null;
if (tableName.equals(TableName.META_TABLE_NAME)){
description = "The hbase:meta table holds references to all User Table regions.";
} else if (tableName.equals(Canary.DEFAULT_WRITE_TABLE_NAME)){
} else if (tableName.equals(CanaryTool.DEFAULT_WRITE_TABLE_NAME)){
description = "The hbase:canary table is used to sniff the write availbility of"
+ " each regionserver.";
} else if (tableName.equals(PermissionStorage.ACL_TABLE_NAME)){
Expand Down
10 changes: 5 additions & 5 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/tool/Canary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@
import org.apache.yetus.audience.InterfaceAudience;

@InterfaceAudience.Public
public interface CanaryInterface {
public interface Canary {

static CanaryInterface create(Configuration conf, ExecutorService executor) {
return new Canary(conf, executor);
static Canary create(Configuration conf, ExecutorService executor) {
return new CanaryTool(conf, executor);
}

@VisibleForTesting
static CanaryInterface create(Configuration conf, ExecutorService executor, Canary.Sink sink) {
return new Canary(conf, executor, sink);
static Canary create(Configuration conf, ExecutorService executor, CanaryTool.Sink sink) {
return new CanaryTool(conf, executor, sink);
}

/**
Expand Down
47 changes: 21 additions & 26 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/tool/CanaryTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
import org.apache.hadoop.hbase.client.Table;
import org.apache.hadoop.hbase.client.TableDescriptor;
import org.apache.hadoop.hbase.filter.FirstKeyOnlyFilter;
import org.apache.hadoop.hbase.tool.Canary.RegionTask.TaskType;
import org.apache.hadoop.hbase.tool.CanaryTool.RegionTask.TaskType;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
import org.apache.hadoop.hbase.util.Pair;
Expand Down Expand Up @@ -121,7 +121,7 @@
* </ol>
*/
@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.TOOLS)
public class Canary implements Tool, CanaryInterface {
public class CanaryTool implements Tool, Canary {

@Override
public int checkRegions(String[] targets) throws Exception {
Expand Down Expand Up @@ -654,6 +654,7 @@ public Void call() {
private static final String CANARY_TABLE_FAMILY_NAME = "Test";

private Configuration conf = null;
private long interval = 0;
private Sink sink = null;

/**
Expand Down Expand Up @@ -688,35 +689,32 @@ public Void call() {
public static final String HBASE_CANARY_ZOOKEEPER_PERMITTED_FAILURES
= "hbase.canary.zookeeper.permitted.failures";

public static final String HBASE_CANARY_INTERVAL = "hbase.canary.interval";
public static final String HBASE_CANARY_TREAT_FAILURE_AS_ERROR
= "hbase.canary.treat.failure.as.error";
public static final String HBASE_CANARY_USE_REGEX = "hbase.canary.use.regex";
public static final String HBASE_CANARY_TIMEOUT = "hbase.canary.timeout";
public static final String HBASE_CANARY_FAIL_ON_ERROR = "hbase.canary.fail.on.error";


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

public Canary() {
public CanaryTool() {
this(new ScheduledThreadPoolExecutor(1));
}

public Canary(ExecutorService executor) {
public CanaryTool(ExecutorService executor) {
this(executor, null);
}

@VisibleForTesting
Canary(ExecutorService executor, Sink sink) {
CanaryTool(ExecutorService executor, Sink sink) {
this.executor = executor;
this.sink = sink;
}

Canary(Configuration conf, ExecutorService executor) {
CanaryTool(Configuration conf, ExecutorService executor) {
this(conf, executor, null);
}

Canary(Configuration conf, ExecutorService executor, Sink sink) {
CanaryTool(Configuration conf, ExecutorService executor, Sink sink) {
this(executor, sink);
setConf(conf);
}
Expand All @@ -736,7 +734,7 @@ public void setConf(Configuration conf) {

private int parseArgs(String[] args) {
int index = -1;
long interval = 0, permittedFailures = 0;
long permittedFailures = 0;
boolean regionServerAllRegions = false, writeSniffing = false;
String readTableTimeoutsStr = null;

Expand All @@ -756,8 +754,7 @@ private int parseArgs(String[] args) {
printUsageAndExit();
} else if (cmd.equals("-daemon") && interval == 0) {
// user asked for daemon mode, set a default interval between checks
conf.setLong(HBASE_CANARY_INTERVAL, DEFAULT_INTERVAL);

interval = DEFAULT_INTERVAL;
} else if (cmd.equals("-interval")) {
// user has specified an interval for canary breaths (-interval N)
i++;
Expand All @@ -773,7 +770,6 @@ private int parseArgs(String[] args) {
System.err.println("-interval needs a numeric value argument.");
printUsageAndExit();
}
conf.setLong(HBASE_CANARY_INTERVAL, interval);
} else if (cmd.equals("-zookeeper")) {
this.zookeeperMode = true;
} else if(cmd.equals("-regionserver")) {
Expand All @@ -785,7 +781,7 @@ private int parseArgs(String[] args) {
writeSniffing = true;
conf.setBoolean(HBASE_CANARY_REGION_WRITE_SNIFFING, true);
} else if(cmd.equals("-treatFailureAsError") || cmd.equals("-failureAsError")) {
conf.setBoolean(HBASE_CANARY_TREAT_FAILURE_AS_ERROR, true);
conf.setBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
} else if (cmd.equals("-e")) {
conf.setBoolean(HBASE_CANARY_USE_REGEX, true);
} else if (cmd.equals("-t")) {
Expand Down Expand Up @@ -931,7 +927,6 @@ private int runMonitor(String[] monitorTargets) throws Exception {
long currentTimeLength = 0;
boolean failOnError = conf.getBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
long timeout = conf.getLong(HBASE_CANARY_TIMEOUT, DEFAULT_TIMEOUT);
long interval = conf.getLong(HBASE_CANARY_INTERVAL, 0);
// Get a connection to use in below.
try (Connection connection = ConnectionFactory.createConnection(this.conf)) {
do {
Expand Down Expand Up @@ -1143,8 +1138,8 @@ private Monitor newMonitor(final Connection connection, String[] monitorTargets)
boolean useRegExp = conf.getBoolean(HBASE_CANARY_USE_REGEX, false);
boolean regionServerAllRegions
= conf.getBoolean(HBASE_CANARY_REGIONSERVER_ALL_REGIONS, false);
boolean treatFailureAsError
= conf.getBoolean(HBASE_CANARY_TREAT_FAILURE_AS_ERROR, false);
boolean failOnError
= conf.getBoolean(HBASE_CANARY_FAIL_ON_ERROR, true);
int permittedFailures
= conf.getInt(HBASE_CANARY_ZOOKEEPER_PERMITTED_FAILURES, 0);
boolean writeSniffing
Expand All @@ -1159,19 +1154,19 @@ private Monitor newMonitor(final Connection connection, String[] monitorTargets)
new RegionServerMonitor(connection, monitorTargets, useRegExp,
getSink(connection.getConfiguration(), RegionServerStdOutSink.class),
this.executor, regionServerAllRegions,
treatFailureAsError, permittedFailures);
failOnError, permittedFailures);

} else if (this.zookeeperMode) {
monitor =
new ZookeeperMonitor(connection, monitorTargets, useRegExp,
getSink(connection.getConfiguration(), ZookeeperStdOutSink.class),
this.executor, treatFailureAsError, permittedFailures);
this.executor, failOnError, permittedFailures);
} else {
monitor =
new RegionMonitor(connection, monitorTargets, useRegExp,
getSink(connection.getConfiguration(), RegionStdOutSink.class),
this.executor, writeSniffing,
TableName.valueOf(writeTableName), treatFailureAsError, configuredReadTableTimeouts,
TableName.valueOf(writeTableName), failOnError, configuredReadTableTimeouts,
configuredWriteTableTimeout, permittedFailures);
}
return monitor;
Expand Down Expand Up @@ -1351,7 +1346,7 @@ public void run() {
this.initialized = true;
for (String table : tables) {
LongAdder readLatency = regionSink.initializeAndGetReadLatencyForTable(table);
taskFutures.addAll(Canary.sniff(admin, regionSink, table, executor, TaskType.READ,
taskFutures.addAll(CanaryTool.sniff(admin, regionSink, table, executor, TaskType.READ,
this.rawScanEnabled, readLatency));
}
} else {
Expand All @@ -1370,7 +1365,7 @@ public void run() {
// sniff canary table with write operation
regionSink.initializeWriteLatency();
LongAdder writeTableLatency = regionSink.getWriteLatency();
taskFutures.addAll(Canary.sniff(admin, regionSink, admin.getDescriptor(writeTableName),
taskFutures.addAll(CanaryTool.sniff(admin, regionSink, admin.getDescriptor(writeTableName),
executor, TaskType.WRITE, this.rawScanEnabled, writeTableLatency));
}

Expand Down Expand Up @@ -1475,7 +1470,7 @@ private List<Future<Void>> sniff(TaskType taskType, RegionStdOutSink regionSink)
(!td.getTableName().equals(writeTableName))) {
LongAdder readLatency =
regionSink.initializeAndGetReadLatencyForTable(td.getTableName().getNameAsString());
taskFutures.addAll(Canary.sniff(admin, sink, td, executor, taskType, this.rawScanEnabled,
taskFutures.addAll(CanaryTool.sniff(admin, sink, td, executor, taskType, this.rawScanEnabled,
readLatency));
}
}
Expand Down Expand Up @@ -1548,7 +1543,7 @@ private static List<Future<Void>> sniff(final Admin admin, final Sink sink, Stri
throws Exception {
LOG.debug("Checking table is enabled and getting table descriptor for table {}", tableName);
if (admin.isTableEnabled(TableName.valueOf(tableName))) {
return Canary.sniff(admin, sink, admin.getDescriptor(TableName.valueOf(tableName)),
return CanaryTool.sniff(admin, sink, admin.getDescriptor(TableName.valueOf(tableName)),
executor, taskType, rawScanEnabled, readLatency);
} else {
LOG.warn("Table {} is not enabled", tableName);
Expand Down Expand Up @@ -1862,7 +1857,7 @@ public static void main(String[] args) throws Exception {
int exitCode;
ExecutorService executor = new ScheduledThreadPoolExecutor(numThreads);
try {
exitCode = ToolRunner.run(conf, new Canary(executor), args);
exitCode = ToolRunner.run(conf, new CanaryTool(executor), args);
} finally {
executor.shutdown();
}
Expand Down
32 changes: 16 additions & 16 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/tool/TestCanaryTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ public void testBasicCanaryWorks() throws Exception {
table.put(p);
}
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
String[] args = { "-writeSniffing", "-t", "10000", tableName.getNameAsString() };
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
assertEquals("verify no read error count", 0, canary.getReadFailures().size());
Expand All @@ -141,8 +141,8 @@ public void testCanaryRegionTaskResult() throws Exception {
table.put(p);
}
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
String[] args = { "-writeSniffing", "-t", "10000", "testCanaryRegionTaskResult" };
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));

Expand All @@ -155,11 +155,11 @@ public void testCanaryRegionTaskResult() throws Exception {

assertTrue("canary should expect to scan at least 1 region",
sink.getTotalExpectedRegions() > 0);
Map<String, Canary.RegionTaskResult> regionMap = sink.getRegionMap();
Map<String, CanaryTool.RegionTaskResult> regionMap = sink.getRegionMap();
assertFalse("verify region map has size > 0", regionMap.isEmpty());

for (String regionName : regionMap.keySet()) {
Canary.RegionTaskResult res = regionMap.get(regionName);
CanaryTool.RegionTaskResult res = regionMap.get(regionName);
assertNotNull("verify each expected region has a RegionTaskResult object in the map", res);
assertNotNull("verify getRegionNameAsString()", regionName);
assertNotNull("verify getRegionInfo()", res.getRegionInfo());
Expand All @@ -168,7 +168,7 @@ public void testCanaryRegionTaskResult() throws Exception {
assertNotNull("verify getServerName()", res.getServerName());
assertNotNull("verify getServerNameAsString()", res.getServerNameAsString());

if (regionName.contains(Canary.DEFAULT_WRITE_TABLE_NAME.getNameAsString())) {
if (regionName.contains(CanaryTool.DEFAULT_WRITE_TABLE_NAME.getNameAsString())) {
assertTrue("write to region " + regionName + " succeeded", res.isWriteSuccess());
assertTrue("write took some time", res.getWriteLatency() > -1);
} else {
Expand All @@ -195,8 +195,8 @@ public void testReadTableTimeouts() throws Exception {
}
}
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
String configuredTimeoutStr = tableNames[0].getNameAsString() + "=" + Long.MAX_VALUE + "," +
tableNames[1].getNameAsString() + "=0";
String[] args = {"-readTableTimeouts", configuredTimeoutStr, name.getMethodName() + "1",
Expand Down Expand Up @@ -225,8 +225,8 @@ public boolean matches(LoggingEvent argument) {
@Test
public void testWriteTableTimeout() throws Exception {
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
String[] args = { "-writeSniffing", "-writeTableTimeout", String.valueOf(Long.MAX_VALUE)};
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
assertNotEquals("verify non-null write latency", null, sink.getWriteLatency());
Expand Down Expand Up @@ -278,8 +278,8 @@ public void testRawScanConfig() throws Exception {
table.put(p);
}
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary.RegionStdOutSink sink = spy(new Canary.RegionStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.RegionStdOutSink sink = spy(new CanaryTool.RegionStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
String[] args = { "-t", "10000", name.getMethodName() };
org.apache.hadoop.conf.Configuration conf =
new org.apache.hadoop.conf.Configuration(testingUtility.getConfiguration());
Expand All @@ -293,7 +293,7 @@ public void testRawScanConfig() throws Exception {

private void runRegionserverCanary() throws Exception {
ExecutorService executor = new ScheduledThreadPoolExecutor(1);
Canary canary = new Canary(executor, new Canary.RegionServerStdOutSink());
CanaryTool canary = new CanaryTool(executor, new CanaryTool.RegionServerStdOutSink());
String[] args = { "-t", "10000", "-regionserver"};
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));
assertEquals("verify no read error count", 0, canary.getReadFailures().size());
Expand All @@ -304,8 +304,8 @@ private void testZookeeperCanaryWithArgs(String[] args) throws Exception {
Iterables.getOnlyElement(testingUtility.getZkCluster().getClientPortList(), null);
testingUtility.getConfiguration().set(HConstants.ZOOKEEPER_QUORUM, "localhost:" + port);
ExecutorService executor = new ScheduledThreadPoolExecutor(2);
Canary.ZookeeperStdOutSink sink = spy(new Canary.ZookeeperStdOutSink());
Canary canary = new Canary(executor, sink);
CanaryTool.ZookeeperStdOutSink sink = spy(new CanaryTool.ZookeeperStdOutSink());
CanaryTool canary = new CanaryTool(executor, sink);
assertEquals(0, ToolRunner.run(testingUtility.getConfiguration(), canary, args));

String baseZnode = testingUtility.getConfiguration()
Expand Down

0 comments on commit 80c3bac

Please sign in to comment.