[GitHub] [accumulo] ctubbsii commented on a change in pull request #1358: Fix race conditions with commitsInProgress counter

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

ctubbsii commented on a change in pull request #1358: Fix race conditions with commitsInProgress counter
URL: https://github.com/apache/accumulo/pull/1358#discussion_r323555558
 
 

 ##########
 File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CommitSession.java
 ##########
 @@ -17,59 +17,73 @@
 package org.apache.accumulo.tserver.tablet;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.apache.accumulo.core.data.Mutation;
 import org.apache.accumulo.core.dataImpl.KeyExtent;
 import org.apache.accumulo.tserver.InMemoryMap;
 import org.apache.accumulo.tserver.log.DfsLogger;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
-public class CommitSession {
+import com.google.common.base.Preconditions;
 
-  private static final Logger log = LoggerFactory.getLogger(CommitSession.class);
+public class CommitSession {
 
   private final long seq;
   private final InMemoryMap memTable;
   private final Tablet committer;
 
-  private int commitsInProgress;
+  private final Lock commitSessionLock = new ReentrantLock();
+  private final Condition noOutstandingCommits =
+      commitSessionLock.newCondition(); 
+
+  private final AtomicInteger commitsInProgress;
   private long maxCommittedTime = Long.MIN_VALUE;
 
   CommitSession(Tablet committer, long seq, InMemoryMap imm) {
     this.seq = seq;
     this.memTable = imm;
     this.committer = committer;
-    commitsInProgress = 0;
+    this.commitsInProgress = new AtomicInteger();
   }
 
   public long getWALogSeq() {
     return seq;
   }
 
   public void decrementCommitsInProgress() {
-    if (commitsInProgress < 1)
-      throw new IllegalStateException("commitsInProgress = " + commitsInProgress);
-
-    commitsInProgress--;
-    if (commitsInProgress == 0)
-      committer.notifyAll();
+    this.commitSessionLock.lock();
+    try {
+      Preconditions.checkState(this.commitsInProgress.get() > 0);
 
 Review comment:
   This represents a loss in debugging information. It's probably better to have the custom message in the `IllegalStateException` for debugging purposes.

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