You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/05/03 18:32:23 UTC

[GitHub] [hbase] shahrs87 opened a new pull request #3222: HBASE-25612 HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

shahrs87 opened a new pull request #3222:
URL: https://github.com/apache/hbase/pull/3222


   This is mainly a back port of [HBASE-23340](https://issues.apache.org/jira/browse/HBASE-23340) / [PR-2779 ](https://github.com/apache/hbase/pull/2779/files) with some test related changes and adding test case.
   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] Apache-HBase commented on pull request #3222: HBASE-25612 HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-831449484


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m  0s |  Docker mode activated.  |
   | -1 :x: |  docker  |   0m  6s |  Docker failed to build yetus/hbase:edccfe439a.  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | GITHUB PR | https://github.com/apache/hbase/pull/3222 |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/1/console |
   | versions | git=2.17.1 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] virajjasani commented on pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-831978375


   Triggered another run


-- 
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:
users@infra.apache.org



[GitHub] [hbase] Reidddddd commented on a change in pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Reidddddd commented on a change in pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#discussion_r627072038



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestLogsCleaner.java
##########
@@ -405,4 +418,77 @@ public RecoverableZooKeeper getRecoverableZooKeeper() {
       return zk;
     }
   }
+
+  /**
+   * An {@link Abortable} implementation for tests.
+   */
+  class TestAbortable implements Abortable {
+    private volatile boolean aborted = false;
+
+    @Override
+    public void abort(String why, Throwable e) {
+      this.aborted = true;
+    }
+
+    @Override
+    public boolean isAborted() {
+      return this.aborted;
+    }
+  }
+
+  /*

Review comment:
       Please use correct java docs style.




-- 
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:
users@infra.apache.org



[GitHub] [hbase] Apache-HBase commented on pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-833426857


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   6m 46s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  The patch appears to include 1 new or modified test files.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 39s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 42s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 45s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 45s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m  3s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 49s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 39s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m  6s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m  3s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 47s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 39s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 39s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 45s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 45s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 35s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   2m 47s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 36s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   3m 55s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 29s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 39s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   2m 56s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  | 135m  6s |  hbase-server in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   0m 38s |  The patch does not generate ASF License warnings.  |
   |  |   | 180m  4s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3222 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux bcf35f9f1665 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3222/out/precommit/personality/provided.sh |
   | git revision | branch-1 / c364443 |
   | Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
   | Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/artifact/out/patch-javac-2.9.2.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/testReport/ |
   | Max. process+thread count | 4623 (vs. ulimit of 10000) |
   | modules | C: hbase-server U: hbase-server |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/4/console |
   | versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] Reidddddd commented on a change in pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Reidddddd commented on a change in pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#discussion_r627072080



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestLogsCleaner.java
##########
@@ -405,4 +418,77 @@ public RecoverableZooKeeper getRecoverableZooKeeper() {
       return zk;
     }
   }
+
+  /**
+   * An {@link Abortable} implementation for tests.
+   */
+  class TestAbortable implements Abortable {
+    private volatile boolean aborted = false;
+
+    @Override
+    public void abort(String why, Throwable e) {
+      this.aborted = true;
+    }
+
+    @Override
+    public boolean isAborted() {
+      return this.aborted;
+    }
+  }
+
+  /*
+   Throw SessionExpiredException when zk#getData is called.
+  */
+  static class SessionExpiredZooKeeperWatcher extends ZooKeeperWatcher {
+    private RecoverableZooKeeper zk;
+
+    public SessionExpiredZooKeeperWatcher(Configuration conf, String identifier,
+                                          Abortable abortable) throws IOException {
+      super(conf, identifier, abortable);
+    }
+
+    public void init() throws Exception {
+      this.zk = spy(super.getRecoverableZooKeeper());
+      doThrow(new KeeperException.SessionExpiredException())
+        .when(zk).getData(Mockito.anyString(), Mockito.any(Watcher.class), Mockito.any(Stat.class));
+    }
+
+    @Override
+    public RecoverableZooKeeper getRecoverableZooKeeper() {
+      return zk;
+    }
+  }
+
+  /*
+  Tests that HMaster#abort will be called if ReplicationLogCleaner

Review comment:
       ditto




-- 
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:
users@infra.apache.org



[GitHub] [hbase] Apache-HBase commented on pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-832128284


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |  11m 40s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  The patch appears to include 1 new or modified test files.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 55s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 41s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 47s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 49s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m  2s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 49s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 41s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m  4s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m  1s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 47s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 39s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 39s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 44s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 44s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 35s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   2m 50s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 29s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   3m 48s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 31s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 41s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   2m 53s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 164m 30s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 39s |  The patch does not generate ASF License warnings.  |
   |  |   | 214m 49s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
   |   | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3222 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux f3ea0b633dec 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3222/out/precommit/personality/provided.sh |
   | git revision | branch-1 / 633d966 |
   | Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
   | Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/testReport/ |
   | Max. process+thread count | 4322 (vs. ulimit of 10000) |
   | modules | C: hbase-server U: hbase-server |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/2/console |
   | versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] Reidddddd commented on a change in pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Reidddddd commented on a change in pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#discussion_r627071005



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java
##########
@@ -132,42 +137,57 @@ public void setConf(Configuration config) {
       LOG.warn("Not configured - allowing all wals to be deleted");
       return;
     }
-    // Make my own Configuration.  Then I'll have my own connection to zk that
-    // I can close myself when comes time.
-    Configuration conf = new Configuration(config);
-    try {
-      setConf(conf, new ZooKeeperWatcher(conf, "replicationLogCleaner", null));
-    } catch (IOException e) {
-      LOG.error("Error while configuring " + this.getClass().getName(), e);
-    }
+    super.setConf(config);
   }
 
-  @InterfaceAudience.Private
-  public void setConf(Configuration conf, ZooKeeperWatcher zk) {
-    super.setConf(conf);
-    try {
-      this.zkw = zk;
-      this.replicationQueues = ReplicationFactory.getReplicationQueuesClient(zkw, conf,
-          new WarnOnlyAbortable());
-      this.replicationQueues.init();
-    } catch (ReplicationException e) {
-      LOG.error("Error while configuring " + this.getClass().getName(), e);
+  @Override
+  public void init(Map<String, Object> params) {
+    if (getConf() == null) {
+      // Replication is disabled so do nothing.
+      return;
+    }
+
+    if (MapUtils.isNotEmpty(params)) {
+      Object master = params.get(HMaster.MASTER);
+      if (master != null && master instanceof HMaster) {
+        this.master = (HMaster)master;
+        zkw = ((HMaster) master).getZooKeeper();
+        shareZK = true;
+      }
     }
+    init(getConf(), this.zkw, null);
   }
 
   @InterfaceAudience.Private
-  public void setConf(Configuration conf, ZooKeeperWatcher zk, 
+  public void init(Configuration conf, ZooKeeperWatcher zk,
       ReplicationQueuesClient replicationQueuesClient) {
     super.setConf(conf);
-    this.zkw = zk;
-    this.replicationQueues = replicationQueuesClient;
+    try {
+      if (zk != null) {
+        this.zkw = zk;
+      } else {
+        this.zkw = new ZooKeeperWatcher(getConf(), "replicationLogCleaner", null);
+      }
+      Preconditions.checkNotNull(this.zkw, "Zookeeper watcher cannot be null");
+      if (replicationQueuesClient != null) {
+        this.replicationQueues = replicationQueuesClient;
+      } else {
+        this.replicationQueues =
+          ReplicationFactory.getReplicationQueuesClient(zkw, getConf(), master);
+        this.replicationQueues.init();
+      }
+      Preconditions.checkNotNull(this.replicationQueues,
+        "ReplicationQueues cannot be null");
+    } catch (IOException | ReplicationException e) {
+      LOG.error("Error while configuring " + this.getClass().getName(), e);

Review comment:
       is only LOG.error enough here?




-- 
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:
users@infra.apache.org



[GitHub] [hbase] Reidddddd merged pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Reidddddd merged pull request #3222:
URL: https://github.com/apache/hbase/pull/3222


   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] shahrs87 commented on a change in pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#discussion_r627357118



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java
##########
@@ -132,42 +137,57 @@ public void setConf(Configuration config) {
       LOG.warn("Not configured - allowing all wals to be deleted");
       return;
     }
-    // Make my own Configuration.  Then I'll have my own connection to zk that
-    // I can close myself when comes time.
-    Configuration conf = new Configuration(config);
-    try {
-      setConf(conf, new ZooKeeperWatcher(conf, "replicationLogCleaner", null));
-    } catch (IOException e) {
-      LOG.error("Error while configuring " + this.getClass().getName(), e);
-    }
+    super.setConf(config);
   }
 
-  @InterfaceAudience.Private
-  public void setConf(Configuration conf, ZooKeeperWatcher zk) {
-    super.setConf(conf);
-    try {
-      this.zkw = zk;
-      this.replicationQueues = ReplicationFactory.getReplicationQueuesClient(zkw, conf,
-          new WarnOnlyAbortable());
-      this.replicationQueues.init();
-    } catch (ReplicationException e) {
-      LOG.error("Error while configuring " + this.getClass().getName(), e);
+  @Override
+  public void init(Map<String, Object> params) {
+    if (getConf() == null) {
+      // Replication is disabled so do nothing.
+      return;
+    }
+
+    if (MapUtils.isNotEmpty(params)) {
+      Object master = params.get(HMaster.MASTER);
+      if (master != null && master instanceof HMaster) {
+        this.master = (HMaster)master;
+        zkw = ((HMaster) master).getZooKeeper();
+        shareZK = true;
+      }
     }
+    init(getConf(), this.zkw, null);
   }
 
   @InterfaceAudience.Private
-  public void setConf(Configuration conf, ZooKeeperWatcher zk, 
+  public void init(Configuration conf, ZooKeeperWatcher zk,
       ReplicationQueuesClient replicationQueuesClient) {
     super.setConf(conf);
-    this.zkw = zk;
-    this.replicationQueues = replicationQueuesClient;
+    try {
+      if (zk != null) {
+        this.zkw = zk;
+      } else {
+        this.zkw = new ZooKeeperWatcher(getConf(), "replicationLogCleaner", null);
+      }
+      Preconditions.checkNotNull(this.zkw, "Zookeeper watcher cannot be null");
+      if (replicationQueuesClient != null) {
+        this.replicationQueues = replicationQueuesClient;
+      } else {
+        this.replicationQueues =
+          ReplicationFactory.getReplicationQueuesClient(zkw, getConf(), master);
+        this.replicationQueues.init();
+      }
+      Preconditions.checkNotNull(this.replicationQueues,
+        "ReplicationQueues cannot be null");
+    } catch (IOException | ReplicationException e) {
+      LOG.error("Error while configuring " + this.getClass().getName(), e);

Review comment:
       I kept the same behavior as before. https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java#L114




-- 
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:
users@infra.apache.org



[GitHub] [hbase] Apache-HBase commented on pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-833602595


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 38s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  The patch appears to include 1 new or modified test files.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 50s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 40s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 45s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 46s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m  6s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 49s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 40s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m  5s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m  3s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 45s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 41s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 41s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 46s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 46s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 39s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   2m 49s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 35s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   3m 49s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 30s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 41s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   2m 56s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  | 142m 13s |  hbase-server in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   0m 37s |  The patch does not generate ASF License warnings.  |
   |  |   | 181m 28s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3222 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux b423cd29e9ef 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3222/out/precommit/personality/provided.sh |
   | git revision | branch-1 / c364443 |
   | Default Java | Azul Systems, Inc.-1.7.0_272-b10 |
   | Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/artifact/out/patch-javac-2.9.2.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/testReport/ |
   | Max. process+thread count | 4337 (vs. ulimit of 10000) |
   | modules | C: hbase-server U: hbase-server |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3222/5/console |
   | versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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:
users@infra.apache.org



[GitHub] [hbase] shahrs87 commented on pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
shahrs87 commented on pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#issuecomment-833536743


   ```
   [INFO] --- maven-surefire-plugin:2.22.2:test (default-test) @ hbase-server ---
   [INFO] 
   [INFO] -------------------------------------------------------
   [INFO]  T E S T S
   [INFO] -------------------------------------------------------
   [INFO] Running org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint
   [INFO] Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 567.874 s - in org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint
   [INFO] Running org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles
   [INFO] Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 568.27 s - in org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles
   [INFO] Running org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFiles
   [INFO] Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 527.033 s - in org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFiles
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 66, Failures: 0, Errors: 0, Skipped: 0
   [INFO] 
   ```
   
   TestSecureLoadIncrementalHFiles, TestLoadIncrementalHFilesUseSecurityEndPoint and TestLoadIncrementalHFiles all succeeding locally.


-- 
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:
users@infra.apache.org



[GitHub] [hbase] shahrs87 commented on a change in pull request #3222: HBASE-25612 [branch-1] HMaster should abort if ReplicationLogCleaner is not able to delete oldWALs.

Posted by GitBox <gi...@apache.org>.
shahrs87 commented on a change in pull request #3222:
URL: https://github.com/apache/hbase/pull/3222#discussion_r627366894



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestLogsCleaner.java
##########
@@ -405,4 +418,77 @@ public RecoverableZooKeeper getRecoverableZooKeeper() {
       return zk;
     }
   }
+
+  /**
+   * An {@link Abortable} implementation for tests.
+   */
+  class TestAbortable implements Abortable {
+    private volatile boolean aborted = false;
+
+    @Override
+    public void abort(String why, Throwable e) {
+      this.aborted = true;
+    }
+
+    @Override
+    public boolean isAborted() {
+      return this.aborted;
+    }
+  }
+
+  /*

Review comment:
       Fixed in latest commit.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestLogsCleaner.java
##########
@@ -405,4 +418,77 @@ public RecoverableZooKeeper getRecoverableZooKeeper() {
       return zk;
     }
   }
+
+  /**
+   * An {@link Abortable} implementation for tests.
+   */
+  class TestAbortable implements Abortable {
+    private volatile boolean aborted = false;
+
+    @Override
+    public void abort(String why, Throwable e) {
+      this.aborted = true;
+    }
+
+    @Override
+    public boolean isAborted() {
+      return this.aborted;
+    }
+  }
+
+  /*
+   Throw SessionExpiredException when zk#getData is called.
+  */
+  static class SessionExpiredZooKeeperWatcher extends ZooKeeperWatcher {
+    private RecoverableZooKeeper zk;
+
+    public SessionExpiredZooKeeperWatcher(Configuration conf, String identifier,
+                                          Abortable abortable) throws IOException {
+      super(conf, identifier, abortable);
+    }
+
+    public void init() throws Exception {
+      this.zk = spy(super.getRecoverableZooKeeper());
+      doThrow(new KeeperException.SessionExpiredException())
+        .when(zk).getData(Mockito.anyString(), Mockito.any(Watcher.class), Mockito.any(Stat.class));
+    }
+
+    @Override
+    public RecoverableZooKeeper getRecoverableZooKeeper() {
+      return zk;
+    }
+  }
+
+  /*
+  Tests that HMaster#abort will be called if ReplicationLogCleaner

Review comment:
       Fixed in latest commit.
   
   




-- 
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:
users@infra.apache.org