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