Apache9 commented on a change in pull request #3849:
URL: https://github.com/apache/hbase/pull/3849#discussion_r771837306
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -133,9 +129,7 @@
import org.apache.hadoop.hbase.util.Threads;
import org.apache.hadoop.hbase.wal.WAL;
import org.apache.hadoop.hbase.wal.WALFactory;
-import org.apache.hadoop.hbase.zookeeper.EmptyWatcher;
-import org.apache.hadoop.hbase.zookeeper.ZKConfig;
-import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.hadoop.hbase.zookeeper.*;
Review comment:
Avoid star imports.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -992,6 +1037,42 @@ public void shutdownMiniCluster() throws IOException {
LOG.info("Minicluster is down");
}
+ /**
+ * Shutdown HBase mini cluster.Does not shutdown zk or dfs if running.
+ * If use external dfs or zk, clean the directory.
+ * @throws java.io.IOException in case command is unsuccessful
+ */
+ public void shutdownMiniHBaseCluster(StartTestingClusterOption option)
throws IOException {
Review comment:
Ditto.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -154,6 +150,8 @@
import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
+import static org.junit.Assert.*;
Review comment:
Ditto. And static imports should be place on top.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -330,10 +329,13 @@ public HBaseTestingUtil(@Nullable Configuration conf) {
}
// Every cluster is a local cluster until we start DFS
// Note that conf could be null, but this.conf will not be
- String dataTestDir = getDataTestDir().toString();
- this.conf.set("fs.defaultFS", "file:///");
- this.conf.set(HConstants.HBASE_DIR, "file://" + dataTestDir);
- LOG.debug("Setting {} to {}", HConstants.HBASE_DIR, dataTestDir);
+ if (this.conf.get("fs.defaultFS") != null &&
!this.conf.get("fs.defaultFS").startsWith("hdfs://")) {
Review comment:
I guess here you do not want to override the fs.defaultFS config if it
is present? But the logic here will skip setting fs.defaultFS if it is not set?
Is this correct?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/StartTestingClusterOption.java
##########
@@ -171,13 +182,22 @@ public boolean isCreateWALDir() {
return createWALDir;
}
+ public boolean isExternalDFS() {
Review comment:
I prefer here we pass in the necessary configs to connecting an external
HDFS, and set them into the Configuration object.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/StartTestingClusterOption.java
##########
@@ -171,13 +182,22 @@ public boolean isCreateWALDir() {
return createWALDir;
}
+ public boolean isExternalDFS() {
+ return externalDFS;
+ }
+
+ public boolean isExternalZK() {
Review comment:
Ditto for zookeeper.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -802,6 +807,24 @@ public SingleProcessHBaseCluster
startMiniCluster(StartTestingClusterOption opti
*/
public SingleProcessHBaseCluster
startMiniHBaseCluster(StartTestingClusterOption option)
throws IOException, InterruptedException {
+
+ if (option.isExternalDFS()) {
+ assertNotNull("fs.defaultFS can not be null, if use external DFS",
conf.get("fs.defaultFS"));
Review comment:
In general I prefer we do not depend on junit methods in HBTU, as it
will force our downstream users also use the same version of junit... But
anyway, not your fault, I think we have already used it in this class...
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
##########
@@ -977,6 +1000,28 @@ public SingleProcessHBaseCluster getMiniHBaseCluster() {
hbaseCluster + " not an instance of " +
SingleProcessHBaseCluster.class.getName());
}
+ /**
+ * Stops mini hbase, zk, and hdfs clusters.
+ * If zk or hdfs is external, clean the znode or dfs path.
+ * @see #startMiniCluster(int)
+ */
+ public void shutdownMiniCluster(StartTestingClusterOption option) throws
IOException {
Review comment:
I think we should know whether we have started DFS cluster and zk by our
own? Or at least we can store the StartTestingClusterOption so users do not
need to pass it again?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
