geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dschnei...@apache.org
Subject [1/2] incubator-geode git commit: modify bits atomically
Date Wed, 11 May 2016 21:51:36 GMT
Repository: incubator-geode
Updated Branches:
  refs/heads/feature/GEODE-1252 [created] 0b8bf230c


modify bits atomically

added unit test for bit methods


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/dfe408a8
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/dfe408a8
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/dfe408a8

Branch: refs/heads/feature/GEODE-1252
Commit: dfe408a885a49dd5dc5a1480593002c41b4362cb
Parents: 3ebcf1f
Author: Darrel Schneider <dschneider@pivotal.io>
Authored: Wed May 11 14:38:09 2016 -0700
Committer: Darrel Schneider <dschneider@pivotal.io>
Committed: Wed May 11 14:38:09 2016 -0700

----------------------------------------------------------------------
 .../internal/cache/versions/VersionTag.java     | 70 +++++++++++----
 .../versions/AbstractVersionTagTestBase.java    | 92 ++++++++++++++++++++
 .../cache/versions/VMVersionTagTest.java        | 32 +++++++
 3 files changed, 178 insertions(+), 16 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/dfe408a8/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/VersionTag.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/VersionTag.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/VersionTag.java
index 60e4299..b51e731 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/VersionTag.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/versions/VersionTag.java
@@ -19,6 +19,7 @@ package com.gemstone.gemfire.internal.cache.versions;
 import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
+import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 import org.apache.logging.log4j.Logger;
 
@@ -74,6 +75,7 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
   private static final int BITS_TIMESTAMP_APPLIED = 0x20;
 
   private static final int BITS_ALLOWED_BY_RESOLVER = 0x40;
+  // Note: the only valid BITS_* are 0xFFFF.
   
   /**
    * the per-entry version number for the operation
@@ -100,10 +102,19 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
    */
   private byte distributedSystemId;
 
+  // In GEODE-1252 we found that the bits field
+  // was concurrently modified by calls to
+  // setPreviousMemberID and setRecorded.
+  // So bits has been changed to volatile and
+  // all modification to it happens using AtomicIntegerFieldUpdater.
+  private static final AtomicIntegerFieldUpdater<VersionTag> bitsUpdater =
+      AtomicIntegerFieldUpdater.newUpdater(VersionTag.class, "bits");
   /**
    * boolean bits
+   * Note: this is an int field so it has 32 bits BUT only the lower 16 bits are serialized.
+   * So all our code should treat this an an unsigned short field.
    */
-  private int bits;
+  private volatile int bits;
 
   /**
    * the initiator of the operation.  If null, the initiator was the sender
@@ -128,7 +139,11 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
 
   /** record that the timestamp from this tag was applied to the cache */
   public void setTimeStampApplied(boolean isTimeStampUpdated) {
-    this.bits |= BITS_TIMESTAMP_APPLIED;
+    if (isTimeStampUpdated) {
+      setBits(BITS_TIMESTAMP_APPLIED);
+    } else {
+      clearBits(~BITS_TIMESTAMP_APPLIED);
+    }
   }
 
   /**
@@ -152,9 +167,9 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
 
   public void setIsGatewayTag(boolean isGateway) {
     if (isGateway) {
-      this.bits = this.bits | BITS_GATEWAY_TAG;
+      setBits(BITS_GATEWAY_TAG);
     } else {
-      this.bits = this.bits & ~BITS_GATEWAY_TAG;
+      clearBits(~BITS_GATEWAY_TAG);
     }
   }
 
@@ -193,7 +208,7 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
    * set that this tag has been recorded in a receiver's RVV
    */
   public void setRecorded() {
-    this.bits |= BITS_RECORDED;
+    setBits(BITS_RECORDED);
   }
 
   /**
@@ -236,7 +251,7 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
    * @param previousMemberID the previousMemberID to set
    */
   public void setPreviousMemberID(T previousMemberID) {
-    this.bits |= BITS_HAS_PREVIOUS_ID;
+    setBits(BITS_HAS_PREVIOUS_ID);
     this.previousMemberID = previousMemberID;
   }
 
@@ -249,9 +264,9 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
    */
   public VersionTag setPosDup(boolean flag) {
     if (flag) {
-      this.bits |= BITS_POSDUP;
+      setBits(BITS_POSDUP);
     } else {
-      this.bits &= ~BITS_POSDUP;
+      clearBits(~BITS_POSDUP);
     }
     return this;
   }
@@ -268,9 +283,9 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
    */
   public VersionTag setAllowedByResolver(boolean flag) {
     if (flag) {
-      this.bits |= BITS_ALLOWED_BY_RESOLVER;
+      setBits(BITS_ALLOWED_BY_RESOLVER);
     } else {
-      this.bits &= ~BITS_ALLOWED_BY_RESOLVER;
+      clearBits(~BITS_ALLOWED_BY_RESOLVER);
     }
     return this;
   }
@@ -331,7 +346,7 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
   public VersionTag(long timestamp, int dsid) {
     this.timeStamp = timestamp;
     this.distributedSystemId = (byte) (dsid & 0xFF);
-    this.bits = BITS_GATEWAY_TAG + BITS_IS_REMOTE_TAG;
+    bitsUpdater.set(this, BITS_GATEWAY_TAG + BITS_IS_REMOTE_TAG);
   }
 
   public void toData(DataOutput out) throws IOException {
@@ -386,7 +401,7 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
     if (logger.isTraceEnabled(LogMarker.VERSION_TAG)) {
       logger.info(LogMarker.VERSION_TAG, "deserializing {} with flags 0x{}", this.getClass(),
Integer.toHexString(flags));
     }
-    this.bits = in.readUnsignedShort();
+    bitsUpdater.set(this, in.readUnsignedShort());
     this.distributedSystemId = in.readByte();
     if ((flags & VERSION_TWO_BYTES) != 0) {
       this.entryVersion = in.readShort() & 0xffff;
@@ -408,11 +423,11 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
         this.previousMemberID = readMember(in);
       }
     }
-    this.bits |= BITS_IS_REMOTE_TAG;
+    setIsRemoteForTesting();
   }
   
   public void setIsRemoteForTesting() {
-    this.bits |= BITS_IS_REMOTE_TAG;
+    setBits(BITS_IS_REMOTE_TAG);
   }
 
   public abstract T readMember(DataInput in) throws IOException, ClassNotFoundException;
@@ -440,14 +455,14 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
       if (this.memberID != null) {
         s.append("; mbr=").append(this.memberID);
       }
-      if ((this.bits & BITS_HAS_PREVIOUS_ID) != 0) {
+      if (hasPreviousMemberID()) {
         s.append("; prev=").append(this.previousMemberID);
       }
       if (this.distributedSystemId >= 0) {
         s.append("; ds=").append(this.distributedSystemId);
       }
       s.append("; time=").append(getVersionTimeStamp());
-      if ((this.bits & BITS_IS_REMOTE_TAG) != 0) {
+      if (isFromOtherMember()) {
         s.append("; remote");
       }
       if (this.isAllowedByResolver()) {
@@ -544,4 +559,27 @@ public abstract class VersionTag<T extends VersionSource> implements
DataSeriali
     }
     return true;
   }
+  
+  /**
+   * Set any bits in the given bitMask on the bits field
+   */
+  private void setBits(int bitMask) {
+    int oldBits;
+    int newBits;
+    do {
+      oldBits = this.bits;
+      newBits = oldBits | bitMask;
+    } while (!bitsUpdater.compareAndSet(this, oldBits, newBits));
+  }
+  /**
+   * Clear any bits not in the given bitMask from the bits field
+   */
+  private void clearBits(int bitMask) {
+    int oldBits;
+    int newBits;
+    do {
+      oldBits = this.bits;
+      newBits = oldBits & bitMask;
+    } while (!bitsUpdater.compareAndSet(this, oldBits, newBits));
+  }
 }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/dfe408a8/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/AbstractVersionTagTestBase.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/AbstractVersionTagTestBase.java
b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/AbstractVersionTagTestBase.java
new file mode 100644
index 0000000..bf0ce43
--- /dev/null
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/AbstractVersionTagTestBase.java
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.gemstone.gemfire.internal.cache.versions;
+
+import static org.junit.Assert.*;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public abstract class AbstractVersionTagTestBase {
+  @SuppressWarnings("rawtypes")
+  protected abstract VersionTag createVersionTag();
+  
+  @SuppressWarnings("rawtypes")
+  private VersionTag vt;
+  
+  @Before
+  public void setup() {
+    this.vt = createVersionTag();
+  }
+  @Test
+  public void testFromOtherMemberBit() {
+    assertEquals(false, vt.isFromOtherMember());
+    vt.setIsRemoteForTesting();
+    assertEquals(true, vt.isFromOtherMember());
+  }
+  
+  @Test
+  public void testTimeStampUpdatedBit() {
+    assertEquals(false, vt.isTimeStampUpdated());
+    vt.setTimeStampApplied(true);
+    assertEquals(true, vt.isTimeStampUpdated());
+    vt.setTimeStampApplied(false);
+    assertEquals(false, vt.isTimeStampUpdated());
+  }
+  
+  @Test
+  public void testGatewayTagBit() {
+    assertEquals(false, vt.isGatewayTag());
+    vt.setIsGatewayTag(true);
+    assertEquals(true, vt.isGatewayTag());
+    vt.setIsGatewayTag(false);
+    assertEquals(false, vt.isGatewayTag());
+  }
+  
+  @Test
+  public void testRecordedBit() {
+    assertEquals(false, vt.isRecorded());
+    vt.setRecorded();
+    assertEquals(true, vt.isRecorded());
+  }
+  
+  @SuppressWarnings("unchecked")
+  @Test
+  public void testPreviousMemberIDBit() {
+    assertEquals(false, vt.hasPreviousMemberID());
+    vt.setPreviousMemberID(null);
+    assertEquals(true, vt.hasPreviousMemberID());
+  }
+  
+  @Test
+  public void testPosDupBit() {
+    assertEquals(false, vt.isPosDup());
+    vt.setPosDup(true);
+    assertEquals(true, vt.isPosDup());
+    vt.setPosDup(false);
+    assertEquals(false, vt.isPosDup());
+  }
+  
+  @Test
+  public void testAllowedByResolverBit() {
+    assertEquals(false, vt.isAllowedByResolver());
+    vt.setAllowedByResolver(true);
+    assertEquals(true, vt.isAllowedByResolver());
+    vt.setAllowedByResolver(false);
+    assertEquals(false, vt.isAllowedByResolver());
+  }
+}

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/dfe408a8/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/VMVersionTagTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/VMVersionTagTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/VMVersionTagTest.java
new file mode 100644
index 0000000..4e39f3d
--- /dev/null
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/versions/VMVersionTagTest.java
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.gemstone.gemfire.internal.cache.versions;
+
+import org.junit.experimental.categories.Category;
+
+import com.gemstone.gemfire.test.junit.categories.UnitTest;
+
+@Category(UnitTest.class)
+public class VMVersionTagTest extends AbstractVersionTagTestBase {
+
+  @SuppressWarnings("rawtypes")
+  @Override
+  protected VersionTag createVersionTag() {
+    return new VMVersionTag();
+  }
+
+}


Mime
View raw message