[GitHub] [hbase] Apache9 commented on a change in pull request #378: HBASE-22684 The log rolling request maybe canceled immediately in Log…

[GitBox <gi...@apache.org>]

Apache9 commented on a change in pull request #378: HBASE-22684 The log rolling request maybe canceled immediately in Log…
URL: https://github.com/apache/hbase/pull/378#discussion_r303240189
 
 

 ##########
 File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
 ##########
 @@ -55,43 +57,40 @@
 @VisibleForTesting
 public class LogRoller extends HasThread implements Closeable {
   private static final Logger LOG = LoggerFactory.getLogger(LogRoller.class);
-  private final ReentrantLock rollLock = new ReentrantLock();
-  private final AtomicBoolean rollLog = new AtomicBoolean(false);
-  private final ConcurrentHashMap<WAL, Boolean> walNeedsRoll = new ConcurrentHashMap<>();
+  private final ConcurrentMap<WAL, Boolean> walNeedsRoll = new ConcurrentHashMap<>();
   private final Server server;
   protected final RegionServerServices services;
   private volatile long lastrolltime = System.currentTimeMillis();
   // Period to roll log.
-  private final long rollperiod;
+  private final long rollPeriod;
   private final int threadWakeFrequency;
   // The interval to check low replication on hlog's pipeline
   private long checkLowReplicationInterval;
 
   private volatile boolean running = true;
 
   public void addWAL(final WAL wal) {
-    if (null == walNeedsRoll.putIfAbsent(wal, Boolean.FALSE)) {
+    if (walNeedsRoll.putIfAbsent(wal, Boolean.FALSE) == null) {
       wal.registerWALActionsListener(new WALActionsListener() {
         @Override
         public void logRollRequested(WALActionsListener.RollRequestReason reason) {
-          walNeedsRoll.put(wal, Boolean.TRUE);
           // TODO logs will contend with each other here, replace with e.g. DelayedQueue
-          synchronized(rollLog) {
-            rollLog.set(true);
-            rollLog.notifyAll();
+          synchronized (LogRoller.this) {
+            walNeedsRoll.put(wal, Boolean.TRUE);
+            LogRoller.this.notifyAll();
           }
         }
       });
     }
   }
 
   public void requestRollAll() {
-    for (WAL wal : walNeedsRoll.keySet()) {
-      walNeedsRoll.put(wal, Boolean.TRUE);
-    }
-    synchronized(rollLog) {
-      rollLog.set(true);
-      rollLog.notifyAll();
+    synchronized (this) {
 
 Review comment:
   It will slow down the throughput but anyway, I do not think there will be much contention, as on one RS even if you enable multi wal, usually we will have less than 10 wals.

----------------------------------------------------------------
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


With regards,
Apache Git Services