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/13 17:54:27 UTC

[GitHub] [hbase] shahrs87 opened a new pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   `TestLogLevel` succeeded locally.
   ```
   [INFO] 
   [INFO] -------------------------------------------------------
   [INFO]  T E S T S
   [INFO] -------------------------------------------------------
   [INFO] Running org.apache.hadoop.hbase.http.log.TestLogLevel
   [INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 9.857 s - in org.apache.hadoop.hbase.http.log.TestLogLevel
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0
   [INFO] 
   [INFO] 
   ``` 
   `hadoopcheck` is failing and is tracked via HBASE-25890
   
   @bharathv  could you please merge the PR ? Thank you !


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   Ran all the failing tests locally.
   ```
   [INFO] -------------------------------------------------------
   [INFO]  T E S T S
   [INFO] -------------------------------------------------------
   [INFO] Running org.apache.hadoop.hbase.procedure.TestFailedProcCleanup
   
   
   [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 79.461 s - in org.apache.hadoop.hbase.procedure.TestFailedProcCleanup
   [INFO] Running org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint
   [INFO] Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 518.231 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: 526.029 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: 515.253 s - in org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFiles
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 68, Failures: 0, Errors: 0, Skipped: 0
   ```
   
   Test*LoadIncrementalHFiles* class is known to be flakey in branch-1. Seen them failing in most of branch-1 PR's.
   


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 14s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |  10m  0s |  branch-1 passed  |
   | +1 :green_heart: |  compile  |   0m 42s |  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 54s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 22s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 18s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 15s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   2m  1s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 42s |  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 45s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m  6s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  hadoopcheck  |   4m 46s |  Patch does not cause any errors with Hadoop 2.8.5 2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 32s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 44s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m 10s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 162m 51s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 33s |  The patch does not generate ASF License warnings.  |
   |  |   | 207m 11s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   |   | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
   
   
   | 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-3264/9/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 5da61e626e7b 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-agent/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / c0ee153 |
   | 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 |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/9/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/9/testReport/ |
   | Max. process+thread count | 5026 (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-3264/9/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 merged pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   ```
   [INFO] 
   [INFO] -------------------------------------------------------
   [INFO]  T E S T S
   [INFO] -------------------------------------------------------
   [INFO] Running org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster
   [INFO] Tests run: 18, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 107.304 s - in org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster
   [INFO] Running org.apache.hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint
   [INFO] Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 587.169 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: 597.287 s - in org.apache.hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles
   [INFO] 
   [INFO] Results:
   [INFO] 
   [INFO] Tests run: 62, Failures: 0, Errors: 0, Skipped: 0
   ```
   All the failed test succeeds on my local machine. 
   @bharathv  could you please review again and merge if okay ? Thank you !


-- 
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] bharathv commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   > master branch doesn't have appendExecutor at all. I am still trying to understand how appends/sync works in master branch.
   
   Ya much of the code has been rewritten with async fshlog. Was curious if there is another manifestation of the same issue.


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       > OTOH, mind handling the return value of awaitTermination()? something like..
   
   @bharathv  Done ! please review again. Thank you !




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   > meanwhile, does this apply to other branches?
   
   @bharathv  This patch doesn't apply to other branches. master branch doesn't have appendExecutor at all. I am still trying to understand how appends/sync works in master branch.


-- 
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 pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   Please hold it, for the release sake. @virajjasani and @bharathv already approved it, I'll do the merge later, no worries about this one.


-- 
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 a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       Although 60s looks reasonable, want to make it configurable? I know another config is pain, but if the time comes, we can tweak it?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       Wondering if we should sleep for 5-10s by default 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] bharathv commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   I'm reading the disruptor shutdown code and now I'm a bit confused, lets hold off on merging this until I get back (by EOD).


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   > Ok, after reading some disruptor code and it's shutdown routine, I think the issue is something else here.
   
   Agree with @bharathv  that the bug lies in appending entries to ringbuffer while disruptor#shutdown is in progress.
   Changed the patch moving setting closed flag before we shutdown the disruptor. That should make sure that we won't add entries to ringbuffer. 
   
   > This bug also applies to branch-2 
   
   Looked at the master and branch-2 code, I don't think both the branches have this bug.
   Below is the shutdown routine in above branches.
   ```
     @Override
     public void shutdown() throws IOException {
       if (!shutdown.compareAndSet(false, true)) {
         return;
       }
       closed = true;  -----> We set the closed flag here.
       // Tell our listeners that the log is closing
       if (!this.listeners.isEmpty()) {
         for (WALActionsListener i : this.listeners) {
           i.logCloseRequested();
         }
       }
       rollWriterLock.lock();
       try {
         doShutdown();  ------> We shutdown the disruptor here.
         if (logArchiveExecutor != null) {
           logArchiveExecutor.shutdownNow();
         }
       } finally {
         rollWriterLock.unlock();
       }
     }
   ```
   [Reference code here.](https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java#L978-L998)
   


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   > > meanwhile, does this apply to other branches?
   > 
   > @bharathv This patch doesn't apply to other branches. master branch doesn't have appendExecutor at all. I am still trying to understand how appends/sync works in master branch.
   
   That's what I was trying to bring up in one of our internal discussion. After bumping Disruptor version on branch-1, our current 
   Disruptor c'tor is deprecated and the one recommended does pass only `ThreadFactory` in place of `Executor`.
   Disruptor 3.4.2 uses their custom internal `BasicExecutor`:
   ```
   public class BasicExecutor implements Executor
   {
       private final ThreadFactory factory;
       private final Queue<Thread> threads = new ConcurrentLinkedQueue<>();
   
       public BasicExecutor(ThreadFactory factory)
       {
           this.factory = factory;
       }
   
       @Override
       public void execute(Runnable command)
       {
           final Thread thread = factory.newThread(command);
           if (null == thread)
           {
               throw new RuntimeException("Failed to create thread to run: " + command);
           }
   
           thread.start();
   
           threads.add(thread);
       }
   
       @Override
       public String toString()
       {
           return "BasicExecutor{" +
               "threads=" + dumpThreadInfo() +
               '}';
       }
   
       private String dumpThreadInfo()
       {
           final StringBuilder sb = new StringBuilder();
   
           final ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
   
           for (Thread t : threads)
           {
               ThreadInfo threadInfo = threadMXBean.getThreadInfo(t.getId());
               sb.append("{");
               sb.append("name=").append(t.getName()).append(",");
               sb.append("id=").append(t.getId()).append(",");
               sb.append("state=").append(threadInfo.getThreadState()).append(",");
               sb.append("lockInfo=").append(threadInfo.getLockInfo());
               sb.append("}");
           }
   
           return sb.toString();
       }
   }
   
   ```


-- 
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] bharathv commented on a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       Can you propagate the interrupt for completeness.
   
   > Wondering if we should sleep for 5-10s by default here.
   
   Curious why?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       Don't have a specific preference on hardcoded/configurability, whatever works for everyone.
   
   OTOH, mind handling the return value of awaitTermination()? something like..
   
   > if (!appendExecutor.awaitTermination(...)) {
   >   LOG.error("Error terminating append pool, potential case for aborted append operations...)
   > }




-- 
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 edited a comment on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

Posted by GitBox <gi...@apache.org>.
shahrs87 edited a comment on pull request #3264:
URL: https://github.com/apache/hbase/pull/3264#issuecomment-845963153


   > Ok, after reading some disruptor code and it's shutdown routine, I think the issue is something else here.
   
   Agree with @bharathv  that the bug lies in appending entries to ringbuffer while disruptor#shutdown is in progress.
   Changed the patch moving setting closed flag before we shutdown the disruptor. That should make sure that we won't add entries to ringbuffer. 
   
   > This bug also applies to branch-2 
   
   Looked at the master and branch-2 code, I don't think both the branches have this bug.
   Below is the shutdown routine in above branches.
   ```
     @Override
     public void shutdown() throws IOException {
       if (!shutdown.compareAndSet(false, true)) {
         return;
       }
       closed = true;  -----> We set the closed flag here.
       // Tell our listeners that the log is closing
       if (!this.listeners.isEmpty()) {
         for (WALActionsListener i : this.listeners) {
           i.logCloseRequested();
         }
       }
       rollWriterLock.lock();
       try {
         doShutdown();  ------> We shutdown the disruptor here.
         if (logArchiveExecutor != null) {
           logArchiveExecutor.shutdownNow();
         }
       } finally {
         rollWriterLock.unlock();
       }
     }
   ```
   [Reference code here.](https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java#L978-L998)
   Cc @apurtell  @virajjasani 


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 11s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 49s |  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 46s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 52s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 18s |  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 42s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 15s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 11s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 56s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 43s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 43s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 47s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 47s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 44s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m  5s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 43s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 12s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 33s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m  6s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 162m  7s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 32s |  The patch does not generate ASF License warnings.  |
   |  |   | 203m 24s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   |   | hadoop.hbase.regionserver.TestSplitTransactionOnCluster |
   
   
   | 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-3264/7/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 2e8f0896c7ee 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/7/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/7/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/7/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/7/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/7/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/7/testReport/ |
   | Max. process+thread count | 4356 (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-3264/7/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 a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       > Although 60s looks reasonable, want to make it configurable? I know another config is pain, but if the time comes, we can tweak it?
   
   @virajjasani  Thank you for the feedback. Added new config key `hbase.regionserver.wal.shutdown.timeout`. Default value to `hbase.regionserver.wal.sync.timeout`.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       > OTOH, mind handling the return value of awaitTermination()? something like..
   
   @bharathv  Done ! please review again. Thank you !

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @bharathv You mean calling Thread.currentThread().interrupt(); in catch block ?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @bharathv You mean calling `Thread.currentThread().interrupt();` in catch block ?




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       Done !




-- 
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] bharathv commented on a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       Right.




-- 
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 a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > Can you propagate the interrupt for completeness.
   
   👍 
   
   > > Wondering if we should sleep for 5-10s by default here.
   > 
   > Curious why?
   
   Since awaiting on termination of Executor did not go as expected, we might want to wait a little more? But now I think we don't need to (I take it back, no explicit wait required) and we are anyways going to interrupt current thread. Instead of waiting, we might rather prefer performing `shutdownNow()` which would interrupt running tasks? (if Disruptor consumer responds to interruption). WDYT?




-- 
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] bharathv commented on a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @virajjasani I don't think we need to shutdownNow() again because the JVM is shutting down anyway and that interrupts all the running threads (if any that exist at that point). If we are in that situation, we are already in a bad state, don't think a proper shutdownNow() would help much.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       Right.




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   @bharathv  @virajjasani  Please don't merge until @Reidddddd  gives a green light. He asked me to [wait here](https://issues.apache.org/jira/browse/HBASE-25890?focusedCommentId=17347283&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17347283).


-- 
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] bharathv commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   Waiting on @virajjasani, he had some questions/comments. 
   @shahrs87 meanwhile, does this apply to other branches?


-- 
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] bharathv commented on a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @virajjasani I don't think we need to shutdownNow() again because the JVM is shutting down anyway and that interrupts all the running threads (if any that exist at that point). If we are in that situation, we are already in a bad state, don't think a proper shutdownNow() would help much.




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 18s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 55s |  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 46s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 52s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 18s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 16s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 11s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 55s |  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 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 43s |  hbase-server: The patch generated 0 new + 48 unchanged - 1 fixed = 48 total (was 49)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m 12s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 44s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 12s |  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  |   3m  5s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 160m 15s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 31s |  The patch does not generate ASF License warnings.  |
   |  |   | 201m 57s |   |
   
   
   | 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-3264/3/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 03df74e59309 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/3/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/3/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/3/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/3/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/3/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/3/testReport/ |
   | Max. process+thread count | 4339 (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-3264/3/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 a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @bharathv You mean calling Thread.currentThread().interrupt(); in catch block ?




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);

Review comment:
       > Although 60s looks reasonable, want to make it configurable? I know another config is pain, but if the time comes, we can tweak it?
   
   @virajjasani  Thank you for the feedback. Added new config key `hbase.regionserver.wal.shutdown.timeout`. Default value to `hbase.regionserver.wal.sync.timeout`.




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |  15m  9s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 52s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 52s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 54s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   2m 16s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 53s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 56s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 46s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 50s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 47s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   2m  8s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 50s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 50s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 53s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 53s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   2m  3s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m 37s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   2m 15s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   5m  8s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 40s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 53s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   4m  9s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 171m  8s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 33s |  The patch does not generate ASF License warnings.  |
   |  |   | 232m 17s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
   |   | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   |   | hadoop.hbase.procedure.TestFailedProcCleanup |
   
   
   | 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-3264/6/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 4f25fb109a07 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/6/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/6/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/6/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/6/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/6/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/6/testReport/ |
   | Max. process+thread count | 4350 (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-3264/6/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] virajjasani edited a comment on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #3264:
URL: https://github.com/apache/hbase/pull/3264#issuecomment-843969438


   > > meanwhile, does this apply to other branches?
   > 
   > @bharathv This patch doesn't apply to other branches. master branch doesn't have appendExecutor at all. I am still trying to understand how appends/sync works in master branch.
   
   That's what I was trying to bring up in one of our internal discussion. After bumping Disruptor version on branch-1, our current 
   Disruptor c'tor is deprecated and the one recommended does pass only `ThreadFactory` in place of `Executor`. That's what is being used by master and branch-2.
   Disruptor 3.4.2 uses their custom internal `BasicExecutor`:
   ```
   public class BasicExecutor implements Executor
   {
       private final ThreadFactory factory;
       private final Queue<Thread> threads = new ConcurrentLinkedQueue<>();
   
       public BasicExecutor(ThreadFactory factory)
       {
           this.factory = factory;
       }
   
       @Override
       public void execute(Runnable command)
       {
           final Thread thread = factory.newThread(command);
           if (null == thread)
           {
               throw new RuntimeException("Failed to create thread to run: " + command);
           }
   
           thread.start();
   
           threads.add(thread);
       }
   
       @Override
       public String toString()
       {
           return "BasicExecutor{" +
               "threads=" + dumpThreadInfo() +
               '}';
       }
   
       private String dumpThreadInfo()
       {
           final StringBuilder sb = new StringBuilder();
   
           final ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
   
           for (Thread t : threads)
           {
               ThreadInfo threadInfo = threadMXBean.getThreadInfo(t.getId());
               sb.append("{");
               sb.append("name=").append(t.getName()).append(",");
               sb.append("id=").append(t.getId()).append(",");
               sb.append("state=").append(threadInfo.getThreadState()).append(",");
               sb.append("lockInfo=").append(threadInfo.getLockInfo());
               sb.append("}");
           }
   
           return sb.toString();
       }
   }
   
   ```


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   @apurtell @bharathv @shahrs87 
   As we had a little chat over Disruptor's constructor (as per 3.4.2 version) when I mentioned about ExecutorService being deprecated from it's constructor argument, on the same front, I just realized that HBase 2 is using recommended (non-deprecated) constructor and not passing ExecutorService and rather just providing ThreadFactory [here](https://github.com/apache/hbase/blob/branch-2/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java#L258-L261)
   
   Wondering how we can ensure that Disruptor's own custom Executor is going to be properly shutdown and not leave any tasks in hanging state when we are just about to close OutputSteam.


-- 
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 edited a comment on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #3264:
URL: https://github.com/apache/hbase/pull/3264#issuecomment-841232549


   @apurtell @bharathv @shahrs87 
   As we had a little chat over Disruptor's constructor (as per 3.4.2 version) when I mentioned about ExecutorService being deprecated from it's constructor argument, on the same front, I just realized that HBase 2 is using recommended (non-deprecated) constructor and not passing ExecutorService and rather just providing ThreadFactory [here](https://github.com/apache/hbase/blob/branch-2/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java#L258-L261)
   
   Wondering how we can ensure that Disruptor's own custom Executor is going to be properly shutdown and not leave any tasks in hanging state when we are just about to close OutputSteam in HBase 2 (HBase 1 is taken care of by @shahrs87's this PR fix)


-- 
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 edited a comment on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

Posted by GitBox <gi...@apache.org>.
shahrs87 edited a comment on pull request #3264:
URL: https://github.com/apache/hbase/pull/3264#issuecomment-844050812


   @bharathv  @virajjasani Thank you for your reviews.  Please don't merge until @Reidddddd  gives a green light. He asked me to [wait here](https://issues.apache.org/jira/browse/HBASE-25890?focusedCommentId=17347283&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17347283).


-- 
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 a change in pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > Can you propagate the interrupt for completeness.
   
   👍 
   
   > > Wondering if we should sleep for 5-10s by default here.
   > 
   > Curious why?
   
   Since awaiting on termination of Executor did not go as expected, we might want to wait a little more? But now I think we don't need to (I take it back, no explicit wait required) and we are anyways going to interrupt current thread. Instead of waiting, we might rather prefer performing `shutdownNow()` at this point, which would interrupt running tasks? (if Disruptor consumer tasks respond to interruption). WDYT?




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |  11m 53s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 52s |  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 53s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 19s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 14s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 10s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 55s |  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 48s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 48s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 43s |  hbase-server: The patch generated 0 new + 48 unchanged - 1 fixed = 48 total (was 49)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m  6s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 46s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 19s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 33s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m  8s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 159m 55s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 34s |  The patch does not generate ASF License warnings.  |
   |  |   | 212m 13s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.client.TestAdmin1 |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   |   | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
   
   
   | 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-3264/4/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 1d06361d915e 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-agent/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/4/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/4/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/4/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/4/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/4/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/4/testReport/ |
   | Max. process+thread count | 4957 (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-3264/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] Apache-HBase commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 17s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 59s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 47s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 49s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 55s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 29s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 50s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 47s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 23s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 19s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 59s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 42s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 49s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 49s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 47s |  hbase-server: The patch generated 0 new + 48 unchanged - 1 fixed = 48 total (was 49)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m 12s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 44s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 11s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 33s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 43s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m 17s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 168m  6s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 33s |  The patch does not generate ASF License warnings.  |
   |  |   | 210m 54s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.mapreduce.TestLoadIncrementalHFiles |
   |   | hadoop.hbase.mapreduce.TestLoadIncrementalHFilesUseSecurityEndPoint |
   |   | hadoop.hbase.mapreduce.TestSecureLoadIncrementalHFiles |
   |   | hadoop.hbase.replication.TestMasterReplication |
   
   
   | 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-3264/2/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 3799982ac6ea 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / 8404470 |
   | 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-3264/2/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/2/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/2/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/2/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/2/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/2/testReport/ |
   | Max. process+thread count | 4364 (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-3264/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] Apache-HBase commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |  12m 29s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 52s |  root in branch-1 failed.  |
   | +1 :green_heart: |  compile  |   0m 44s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  compile  |   0m 50s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 57s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 25s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 51s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 44s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 16s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 12s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   2m  0s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 42s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 48s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 48s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 48s |  hbase-server: The patch generated 0 new + 48 unchanged - 1 fixed = 48 total (was 49)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m 11s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 44s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 14s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 34s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m 46s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 166m 17s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 36s |  The patch does not generate ASF License warnings.  |
   |  |   | 220m 34s |   |
   
   
   | 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-3264/1/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 0f3d0b59c2ff 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / 8404470 |
   | 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-3264/1/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/1/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/1/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/1/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/1/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/1/testReport/ |
   | Max. process+thread count | 4411 (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-3264/1/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] bharathv commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   > That's what I was trying to bring up in one of our internal discussion. After bumping Disruptor version on branch-1, our current
   Disruptor c'tor is deprecated and the one recommended does pass only ThreadFactory in place of Executor. That's what is being used by master and branch-2.
   Disruptor 3.4.2 uses their custom internal BasicExecutor:
   
   @virajjasani I think the bug is something else here. See my last comment. Its weird that disruptor is not closing that executor, I don't know why. In the deprecated version of c'tor we atleast have access to the executor to shut it down but doesn't seem like the case in the later versions. They explicitly [state](https://github.com/LMAX-Exchange/disruptor/blob/46f57d94a188c2d9347e2aa0975e20332b0ae39a/src/main/java/com/lmax/disruptor/dsl/Disruptor.java#L425) that they don't shutdown the executor.


-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -1120,8 +1120,18 @@ public void shutdown() throws IOException {
           this.disruptor.shutdown();
         }
       }
-      // With disruptor down, this is safe to let go.
-      if (this.appendExecutor !=  null) this.appendExecutor.shutdown();
+
+      try {
+        // With disruptor down, this is safe to let go.
+        if (this.appendExecutor != null) {
+          this.appendExecutor.shutdown();
+          // Wait for 60 seconds to complete currently running tasks.
+          this.appendExecutor.awaitTermination(60, TimeUnit.SECONDS);
+        }
+      } catch (InterruptedException e) {
+        LOG.warn("Exception while waiting to shutdown executor ", e);

Review comment:
       > @shahrs87 I think you missed the comment on resetting the interrupt flag.
   
   @bharathv You mean calling `Thread.currentThread().interrupt();` in catch block ?




-- 
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 #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m  9s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 48s |  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 47s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 56s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 21s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 43s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 13s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m  9s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 55s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 43s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 43s |  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 42s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  1s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m  5s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 46s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 10s |  The patch causes 10 errors with Hadoop v2.9.2.  |
   | +1 :green_heart: |  javadoc  |   0m 32s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m  6s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  | 162m 49s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 33s |  The patch does not generate ASF License warnings.  |
   |  |   | 203m 54s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | 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-3264/8/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux f3ba8a786a3f 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/8/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/8/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/8/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/8/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/8/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/8/testReport/ |
   | Max. process+thread count | 4266 (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-3264/8/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] Apache-HBase commented on pull request #3264: HBASE-25887 Wait for appendExecutor to shutdown running tasks

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


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 18s |  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.  |
   | -0 :warning: |  test4tests  |   0m  0s |  The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.  |
   ||| _ branch-1 Compile Tests _ |
   | -1 :x: |  mvninstall  |   9m 51s |  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 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  checkstyle  |   1m 57s |  branch-1 passed  |
   | +1 :green_heart: |  shadedjars  |   3m 23s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 48s |  branch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javadoc  |   0m 44s |  branch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +0 :ok: |  spotbugs  |   3m 16s |  Used deprecated FindBugs config; considering switching to SpotBugs.  |
   | +1 :green_heart: |  findbugs  |   3m 14s |  branch-1 passed  |
   ||| _ Patch Compile Tests _ |
   | -1 :x: |  mvninstall  |   1m 56s |  root in the patch failed.  |
   | +1 :green_heart: |  compile  |   0m 42s |  the patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19  |
   | +1 :green_heart: |  javac  |   0m 42s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 47s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  javac  |   0m 47s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   1m 43s |  hbase-server: The patch generated 0 new + 48 unchanged - 1 fixed = 48 total (was 49)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  shadedjars  |   3m  6s |  patch has no errors when building our shaded downstream artifacts.  |
   | -1 :x: |  hadoopcheck  |   1m 46s |  The patch causes 10 errors with Hadoop v2.8.5.  |
   | -1 :x: |  hadoopcheck  |   4m 12s |  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 43s |  the patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10  |
   | +1 :green_heart: |  findbugs  |   3m  7s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  |  35m 41s |  hbase-server in the patch failed.  |
   | +1 :green_heart: |  asflicense  |   0m 24s |  The patch does not generate ASF License warnings.  |
   |  |   |  77m 25s |   |
   
   
   | Reason | Tests |
   |-------:|:------|
   | Failed junit tests | hadoop.hbase.http.log.TestLogLevel |
   
   
   | 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-3264/5/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/3264 |
   | Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
   | uname | Linux 56159e406ad1 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | /home/jenkins/jenkins-agent/workspace/Base-PreCommit-GitHub-PR_PR-3264/out/precommit/personality/provided.sh |
   | git revision | branch-1 / ddac801 |
   | 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-3264/5/artifact/out/branch-mvninstall-root.txt |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/5/artifact/out/patch-mvninstall-root.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/5/artifact/out/patch-javac-2.8.5.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/5/artifact/out/patch-javac-2.9.2.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/5/artifact/out/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3264/5/testReport/ |
   | Max. process+thread count | 734 (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-3264/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