geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kl...@apache.org
Subject [01/16] incubator-geode git commit: GEDOE-93: Entry count stats are incorrect with PR with entry eviction and async disk
Date Sat, 18 Jun 2016 21:05:47 GMT
Repository: incubator-geode
Updated Branches:
  refs/heads/feature/GEODE-1565 ca5b45cc8 -> db739b221


GEDOE-93: Entry count stats are incorrect with PR with entry eviction and async disk

 * Refactored disk status update logic to do it at one place
 * Lazily updating stats for disk async case to avoid complex
   logic to undo stats in case if there is another operation
   before the data is flushed to disk
 * Fixed a wan test and wrapped the asserts inside awaitility


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

Branch: refs/heads/feature/GEODE-1565
Commit: 8a132221fcfe6d2e2e8ba97628307c5fd993c047
Parents: 3dd2efd
Author: Sai Boorlagadda <sboorlagadda@pivotal.io>
Authored: Mon May 23 13:10:47 2016 -0700
Committer: Sai Boorlagadda <sboorlagadda@pivotal.io>
Committed: Thu Jun 16 11:46:34 2016 -0700

----------------------------------------------------------------------
 .../gemfire/internal/cache/DiskEntry.java       | 88 +++++++++++---------
 .../gemfire/internal/cache/LocalRegion.java     |  4 -
 .../cache/PartitionedRegionStatsJUnitTest.java  | 26 ++----
 ...llelGatewaySenderQueueOverflowDUnitTest.java | 42 ++++++----
 4 files changed, 76 insertions(+), 84 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/8a132221/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java
index 5da0d9a..698e3bd 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DiskEntry.java
@@ -897,30 +897,31 @@ public interface DiskEntry extends RegionEntry {
           did.setUserBits(re.getUserBits());
           oldValueLength = did.getValueLength();
           did.setValueLength(re.getValueLength());
-          // The following undo and then do fixes bug 41849
-          // First, undo the stats done for the previous recovered value
-          if (oldKeyId < 0) {
-            dr.incNumOverflowOnDisk(-1L);
-            dr.incNumOverflowBytesOnDisk(-oldValueLength);
-            incrementBucketStats(region, 0/*InVM*/, -1/*OnDisk*/, -oldValueLength);
-          } else {
-            dr.incNumEntriesInVM(-1L);
-            incrementBucketStats(region, -1/*InVM*/, 0/*OnDisk*/, 0);
-          }
-          // Second, do the stats done for the current recovered value
+          
           if (re.getRecoveredKeyId() < 0) {
             if (!entry.isValueNull()) {
               entry.handleValueOverflow(region);
               entry.setValueWithContext(region, null); // fixes bug 41119
             }
-            dr.incNumOverflowOnDisk(1L);
-            dr.incNumOverflowBytesOnDisk(did.getValueLength());
-            incrementBucketStats(region, 0/*InVM*/, 1/*OnDisk*/,
-                                 did.getValueLength());
           } else {
             entry.setValueWithContext(region, entry.prepareValueForCache(region, re.getValue(),
false));
-            dr.incNumEntriesInVM(1L);
-            incrementBucketStats(region, 1/*InVM*/, 0/*OnDisk*/, 0);
+          }
+          
+          if (re.getRecoveredKeyId() < 0) {
+            if(oldKeyId >= 0) {
+              dr.incNumEntriesInVM(-1L);
+              dr.incNumOverflowOnDisk(1L);
+              dr.incNumOverflowBytesOnDisk(did.getValueLength());
+              incrementBucketStats(region, -1/*InVM*/, 1/*OnDisk*/,
+                                   did.getValueLength());
+            }
+          } else {
+            if(oldKeyId < 0) {
+              dr.incNumEntriesInVM(1L);
+              dr.incNumOverflowOnDisk(-1L);
+              dr.incNumOverflowBytesOnDisk(-oldValueLength);
+              incrementBucketStats(region, 1/*InVM*/, -1/*OnDisk*/, -oldValueLength);
+            }
           }
         }
         else {
@@ -993,24 +994,33 @@ public interface DiskEntry extends RegionEntry {
             dr.incNumEntriesInVM(1L);
             incrementBucketStats(region, 1/*InVM*/, 0/*OnDisk*/, 0);
           }
-        }
-        if (entry instanceof LRUEntry) {
-          LRUEntry le = (LRUEntry)entry;
-          boolean wasEvicted = le.testEvicted();
-          le.unsetEvicted();
-          if (!Token.isRemovedFromDisk(newValue)) {
-            if (oldValue == null
-                // added null check for bug 41759
-                || wasEvicted && did != null && did.isPendingAsync()) {
-              // Note we do not append this entry because that will be
-              // done by lruEntryUpdate
+          
+          if(newValue == Token.TOMBSTONE) {
+            if (oldValue == null) {
+              dr.incNumOverflowOnDisk(-1L);
+              dr.incNumOverflowBytesOnDisk(-oldValueLength);
+              incrementBucketStats(region, 0/*InVM*/, -1/*OnDisk*/, -oldValueLength);
+            } else {
+              dr.incNumEntriesInVM(-1L);
+              incrementBucketStats(region, -1/*InVM*/, 0/*OnDisk*/, 0);
+            }
+          } else {
+            if (oldValue == null) {
               dr.incNumEntriesInVM(1L);
               dr.incNumOverflowOnDisk(-1L);
               dr.incNumOverflowBytesOnDisk(-oldValueLength);
               incrementBucketStats(region, 1/*InVM*/, -1/*OnDisk*/, -oldValueLength);
+            } else if(oldValue == Token.TOMBSTONE) {
+              dr.incNumEntriesInVM(1L);
+              incrementBucketStats(region, 1/*InVM*/, 0/*OnDisk*/, 0/*overflowBytesOnDisk*/);
             }
           }
         }
+        if (entry instanceof LRUEntry) {
+          LRUEntry le = (LRUEntry)entry;
+          le.unsetEvicted();          
+        }
+
       }
       } finally {
         if (syncObj == did) {
@@ -1158,11 +1168,6 @@ public interface DiskEntry extends RegionEntry {
                 // Seems like we could end up adding it to the queue multiple times.
                 did.setPendingAsync(false);
               }
-              // since it was evicted fix the stats here
-              dr.incNumEntriesInVM(1L);
-              dr.incNumOverflowOnDisk(-1L);
-              // no need to dec overflowBytesOnDisk because it was not inced in this case.
-              incrementBucketStats(region, 1/*InVM*/, -1/*OnDisk*/, 0);
             }
             lruEntryFaultIn((LRUEntry) entry, region);
             lruFaultedIn = true;
@@ -1445,7 +1450,6 @@ public interface DiskEntry extends RegionEntry {
             // and now we are faulting it out
           }
         }
-
         boolean movedValueToDisk = false; // added for bug 41849
         
         // If async then if it does not need to be written (because it already was)
@@ -1465,10 +1469,12 @@ public interface DiskEntry extends RegionEntry {
         if (movedValueToDisk) {
           valueLength = getValueLength(did);
         }
-        dr.incNumEntriesInVM(-1L);
-        dr.incNumOverflowOnDisk(1L);
-        dr.incNumOverflowBytesOnDisk(valueLength);
-        incrementBucketStats(region, -1/*InVM*/, 1/*OnDisk*/, valueLength);
+        if(dr.isSync() || movedValueToDisk) {
+          dr.incNumEntriesInVM(-1L);
+          dr.incNumOverflowOnDisk(1L);
+          dr.incNumOverflowBytesOnDisk(valueLength);
+          incrementBucketStats(region, -1/*InVM*/, 1/*OnDisk*/, valueLength);
+        }
       }
       } finally {
         dr.releaseReadLock();
@@ -1650,10 +1656,10 @@ public interface DiskEntry extends RegionEntry {
                   && ((LRUEntry)entry).testEvicted()) {
                 // Moved this here to fix bug 40116.
                 region.updateSizeOnEvict(entry.getKey(), entryValSize);
-                // note the old size was already accounted for
-                // onDisk was already inced so just do the valueLength here
+                dr.incNumEntriesInVM(-1);
+                dr.incNumOverflowOnDisk(1L);
                 dr.incNumOverflowBytesOnDisk(did.getValueLength());
-                incrementBucketStats(region, 0/*InVM*/, 0/*OnDisk*/,
+                incrementBucketStats(region, -1/*InVM*/, 1/*OnDisk*/,
                                      did.getValueLength());
                 entry.handleValueOverflow(region);
                 entry.setValueWithContext(region,null);

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/8a132221/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java
b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java
index e5897cc..205f38f 100644
--- a/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java
@@ -3280,10 +3280,6 @@ public class LocalRegion extends AbstractRegion
     //Fix for 45204 - don't include the tombstones in
     //any of our entry count stats.
     this.cachePerfStats.incEntryCount(-delta);
-    if(getDiskRegion() != null) {
-      getDiskRegion().incNumEntriesInVM(-delta);
-    }
-    DiskEntry.Helper.incrementBucketStats(this, -delta/*InVM*/, 0/*OnDisk*/, 0);
   }
   
   public int getTombstoneCount() {

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/8a132221/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/PartitionedRegionStatsJUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/PartitionedRegionStatsJUnitTest.java
b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/PartitionedRegionStatsJUnitTest.java
index 1a3277c..82e3489 100644
--- a/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/PartitionedRegionStatsJUnitTest.java
+++ b/geode-core/src/test/java/com/gemstone/gemfire/internal/cache/PartitionedRegionStatsJUnitTest.java
@@ -475,30 +475,14 @@ public class PartitionedRegionStatsJUnitTest
     assertEquals(singleEntryMemSize * entriesInMem, stats.getLong("dataStoreBytesInUse"));
     assertEquals(numEntries , stats.getInt("dataStoreEntryCount"));
     assertEquals((numEntries - entriesInMem) * entryOverflowSize, diskStats.getNumOverflowBytesOnDisk());
-    //Disabled for GEODE-93. numEntriesInVM and numOVerflowOnDisk are incorrect
-//    assertIndexDetailsEquals(entriesInMem , diskStats.getNumEntriesInVM());
-//    assertIndexDetailsEquals((numEntries - entriesInMem) , diskStats.getNumOverflowOnDisk());
-      assertEquals(stats.getLong("dataStoreBytesInUse"), getMemBytes(pr));
-      assertEquals(diskStats.getNumOverflowBytesOnDisk(), getDiskBytes(pr));
-    }
-
-  private int countEntriesInMem(PartitionedRegion pr) {
-    int entriesInMem = 0;
-    for(BucketRegion br : pr.getDataStore().getAllLocalBucketRegions()) {
-      for(RegionEntry entry : br.entries.regionEntries()) {
-        if(entry._getValue() != null && !Token.isRemoved(entry._getValue())) {
-          System.out.println("Still in memory " + entry.getKey());
-          entriesInMem++;
-        }
-      }
-    }
-    
-    System.out.println("EntriesInMem = " + entriesInMem);
-    return entriesInMem;
+    assertEquals(entriesInMem , diskStats.getNumEntriesInVM());
+    assertEquals((numEntries - entriesInMem) , diskStats.getNumOverflowOnDisk());
+    assertEquals(stats.getLong("dataStoreBytesInUse"), getMemBytes(pr));
+    assertEquals(diskStats.getNumOverflowBytesOnDisk(), getDiskBytes(pr));
   }
 
   private Object getDiskBytes(PartitionedRegion pr) {
-Set<BucketRegion> brs = pr.getDataStore().getAllLocalBucketRegions();
+    Set<BucketRegion> brs = pr.getDataStore().getAllLocalBucketRegions();
     
     long bytes = 0;
     for(Iterator<BucketRegion> itr = brs.iterator(); itr.hasNext(); ) {

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/8a132221/geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/parallel/ParallelGatewaySenderQueueOverflowDUnitTest.java
----------------------------------------------------------------------
diff --git a/geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/parallel/ParallelGatewaySenderQueueOverflowDUnitTest.java
b/geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/parallel/ParallelGatewaySenderQueueOverflowDUnitTest.java
index e2fe10b..44308f8 100644
--- a/geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/parallel/ParallelGatewaySenderQueueOverflowDUnitTest.java
+++ b/geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/parallel/ParallelGatewaySenderQueueOverflowDUnitTest.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.*;
 import java.io.File;
 import java.util.Properties;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import org.junit.Ignore;
 import org.junit.Test;
@@ -49,6 +50,7 @@ import com.gemstone.gemfire.test.dunit.LogWriterUtils;
 import com.gemstone.gemfire.test.dunit.VM;
 import com.gemstone.gemfire.test.dunit.Wait;
 import com.gemstone.gemfire.test.junit.categories.DistributedTest;
+import com.jayway.awaitility.Awaitility;
 
 /**
  * DUnit for ParallelSenderQueue overflow operations.
@@ -98,26 +100,30 @@ public class ParallelGatewaySenderQueueOverflowDUnitTest extends WANTestBase
{
     int numEventPuts = 50;
     vm4.invoke(() -> WANTestBase.doHeavyPuts( getTestMethodName(), numEventPuts ));
     
-    long numOvVm4 = (Long) vm4.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
-    long numOvVm5 = (Long) vm5.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
-    long numOvVm6 = (Long) vm6.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
-    long numOvVm7 = (Long) vm7.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
-    
-    long numMemVm4 = (Long) vm4.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
-    long numMemVm5 = (Long) vm5.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
-    long numMemVm6 = (Long) vm6.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
-    long numMemVm7 = (Long) vm7.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
     
-    LogWriterUtils.getLogWriter().info("Entries overflown to disk: " + numOvVm4 + "," + numOvVm5
+ "," + numOvVm6 + "," + numOvVm7);
-    LogWriterUtils.getLogWriter().info("Entries in VM: " + numMemVm4 + "," + numMemVm5 +
"," + numMemVm6 + "," + numMemVm7);
-    
-    long totalOverflown = numOvVm4 + numOvVm5 + numOvVm6 + numOvVm7; 
     //considering a memory limit of 40 MB, maximum of 40 events can be in memory. Rest should
be on disk.
-    assertTrue("Total number of entries overflown to disk should be at least greater than
55", (totalOverflown > 55));
-    
-    long totalInMemory = numMemVm4 + numMemVm5 + numMemVm6 + numMemVm7;
-    //expected is twice the number of events put due to redundancy level of 1  
-    assertEquals("Total number of entries on disk and in VM is incorrect", (numEventPuts*2),
(totalOverflown + totalInMemory));
+    Awaitility.await().atMost(60, TimeUnit.SECONDS).until(()-> 
+    {
+      long numOvVm4 = (Long) vm4.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
+      long numOvVm5 = (Long) vm5.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
+      long numOvVm6 = (Long) vm6.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
+      long numOvVm7 = (Long) vm7.invoke(() -> WANTestBase.getNumberOfEntriesOverflownToDisk(
"ln" ));
+      
+      long numMemVm4 = (Long) vm4.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
+      long numMemVm5 = (Long) vm5.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
+      long numMemVm6 = (Long) vm6.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
+      long numMemVm7 = (Long) vm7.invoke(() -> WANTestBase.getNumberOfEntriesInVM( "ln"
));
+      
+      LogWriterUtils.getLogWriter().info("Entries overflown to disk: " + numOvVm4 + "," +
numOvVm5 + "," + numOvVm6 + "," + numOvVm7);
+      LogWriterUtils.getLogWriter().info("Entries in VM: " + numMemVm4 + "," + numMemVm5
+ "," + numMemVm6 + "," + numMemVm7);
+      long totalOverflown = numOvVm4 + numOvVm5 + numOvVm6 + numOvVm7; 
+      assertTrue("Total number of entries overflown to disk should be at least greater than
55", (totalOverflown > 55));
+
+      long totalInMemory = numMemVm4 + numMemVm5 + numMemVm6 + numMemVm7;
+      //expected is twice the number of events put due to redundancy level of 1  
+      assertEquals("Total number of entries on disk and in VM is incorrect", (numEventPuts*2),
(totalOverflown + totalInMemory));
+      
+    });
     
     vm4.invoke(() -> WANTestBase.resumeSender( "ln" ));
     vm5.invoke(() -> WANTestBase.resumeSender( "ln" ));


Mime
View raw message