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

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

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

 ##########
 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:
   For this type of debugging, `Preconditions` is a helpful tool to put up bumpers to prevent other developers from interacting with a method it wasn't meant to be.  This is not something that is going to be useful as an error for end-users.  With that in mind, any developer can use the stack trace to find the issue.

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