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

 ##########
 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) {
+      List<WAL> wals = new ArrayList<WAL>(walNeedsRoll.keySet());
+      for (WAL wal : wals) {
+        walNeedsRoll.put(wal, Boolean.TRUE);
 
 Review comment:
   Ain't the lock checked on each append to the WAL though? (IIRC?)

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


With regards,
Apache Git Services

Reply via email to