[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5978: GEODE-8894 allow individual deltas to trigger bucket size recalculation

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

dschneider-pivotal commented on a change in pull request #5978:
URL: https://github.com/apache/geode/pull/5978#discussion_r570517145



##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -55,7 +55,20 @@
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
 
   /**
-   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied
+   * By default, buckets do not recalculate their size when deltas are applied. This optimizes for
+   * the case where the sie of an entry does not change. However, if the size does increase or

Review comment:
       typo: "sie" -> "size"

##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -55,7 +55,20 @@
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
 
   /**
-   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied
+   * By default, buckets do not recalculate their size when deltas are applied. This optimizes for
+   * the case where the sie of an entry does not change. However, if the size does increase or
+   * decrease, this default behavior can result in the memory usage statistics becoming inaccurate.
+   *
+   * There is a global Geode property, DELTAS_RECALCULATE_SIZE, which can be used to cause all
+   * deltas to trigger bucket size recalculation when deltas are applied. By default, this is set
+   * to 'false' because of potential performance impacts when every delta triggers a recalculation.
+   *
+   * This method allows bucket size recalculation on a per-delta basis. For example, geode-redis

Review comment:
       Once again I would not refer to bucket size in this context. Changing "bucket" to "delta" here makes sense. You could add a section that says what difference having an accurate delta size makes. You could mention that geode has statistics that track memory usage and that geode attempts to keep a partitioned region's memory usage balanced across the cluster using the size.

##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -55,7 +55,20 @@
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
 
   /**
-   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied
+   * By default, buckets do not recalculate their size when deltas are applied. This optimizes for
+   * the case where the sie of an entry does not change. However, if the size does increase or
+   * decrease, this default behavior can result in the memory usage statistics becoming inaccurate.
+   *
+   * There is a global Geode property, DELTAS_RECALCULATE_SIZE, which can be used to cause all

Review comment:
       I think it might be confusing the calls this a "Geode property" because it might get confused with the gemfire properties. This is actually a java system property that Geode reads. And the actual property is "gemfire.DELTAS_RECALCULATE_SIZE".

##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -55,7 +55,20 @@
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
 
   /**
-   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied
+   * By default, buckets do not recalculate their size when deltas are applied. This optimizes for

Review comment:
       Instead of "buckets" I would focus this on deltas. So say something like: "By default, the size of an entry value that implements the Delta interface is not recalculated when the delta changes.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
##########
@@ -1719,7 +1716,7 @@ private void setNewValueInRegion(final InternalRegion owner, final RegionEntry r
       int vSize;
       Object ov = basicGetOldValue();
       if (ov instanceof CachedDeserializable && !(GemFireCacheImpl.DELTAS_RECALCULATE_SIZE
-          || this.forceRecalculateSize)) {
+          || ((org.apache.geode.Delta) v).getForceRecalculateSize())) {

Review comment:
       Would it be possible to import Delta so we these instanceof and casts get a bit shorter?

##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -55,7 +55,20 @@
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
 
   /**
-   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied
+   * By default, buckets do not recalculate their size when deltas are applied. This optimizes for
+   * the case where the sie of an entry does not change. However, if the size does increase or
+   * decrease, this default behavior can result in the memory usage statistics becoming inaccurate.
+   *
+   * There is a global Geode property, DELTAS_RECALCULATE_SIZE, which can be used to cause all
+   * deltas to trigger bucket size recalculation when deltas are applied. By default, this is set
+   * to 'false' because of potential performance impacts when every delta triggers a recalculation.
+   *
+   * This method allows bucket size recalculation on a per-delta basis. For example, geode-redis
+   * uses deltas heavily, and many commands (e.g. APPEND) cause the size of the entry to change.
+   * By overriding this method to return 'true', Redis memory usage statistics can be kept accurate

Review comment:
       I recommend that you remove the redis example stuff from this section.




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