ramkrish86 commented on a change in pull request #2168:
URL: https://github.com/apache/hbase/pull/2168#discussion_r462191066
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -436,6 +464,19 @@ protected void doShutdown() throws IOException {
this.writer.close();
this.writer = null;
}
+ closeExecutor.shutdown();
+ try {
+ if (!closeExecutor.awaitTermination(waitOnShutdownInSeconds,
TimeUnit.SECONDS)) {
+ LOG.error("We have waited " + waitOnShutdownInSeconds + " seconds but"
Review comment:
May be we can replace with that {} method of logging where we have the
placeholders?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
##########
@@ -412,6 +422,24 @@ protected void doReplaceWriter(Path oldPath, Path newPath,
Writer nextWriter) th
}
}
+ private void closeWriter(Path path, boolean syncCloseCall) throws
IOException {
+ try {
+ TraceUtil.addTimelineAnnotation("closing writer");
+ writer.close();
+ TraceUtil.addTimelineAnnotation("writer closed");
+ } catch (IOException ioe) {
+ int errors = closeErrorCount.incrementAndGet();
+ boolean hasUnflushedEntries = isUnflushedEntries();
+ if (syncCloseCall && (hasUnflushedEntries || (errors >
this.closeErrorsTolerated))) {
Review comment:
confirming once again with hasFlushedEntries when syncCloseCall is it
mandatory or just to be on the safe side? Because even if hasUnFlushedEntries
is false here i think errors would have crossed the limit anyway because we
increment and then check it.
Rest LGTM. Can check the QA once. May be you may have to change some cluster
based test to run with FSHLog as the default may be AsyncWAL.
----------------------------------------------------------------
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:
[email protected]
