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



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
##########
@@ -165,6 +165,9 @@
    */
   private byte[] deltaBytes = null;
 
+  /** If true, causes deltas to trigger recalculation of bucket size **/
+  private boolean forceRecalculateSize = false;

Review comment:
       I'd like to see this new variable removed and instead, in the two places we use it, call delta.getForceRecalculateSize().
   You can easily do this on line 1722 because it had just done (on line 1718) a test that v is an instanceof Delta. So add the next line: Delta delta = (Delta)v;
   
   On line 1865 the delta instance is the "value" local variable. You can see this on line 1841 when this code casts "value" to Delta. So I'd recommend you add a local variable at 1841 like so: Delta delta = (Delta)value;
   and then use that on 1865 to decide if we should recalculate.
   Having the extra state on EntryEventImpl adds complexity and in the future it could get out of sync with Delta.getForceRecalculateSize(). By just calling this new method I think the code will be better.

##########
File path: geode-core/src/main/java/org/apache/geode/Delta.java
##########
@@ -53,7 +51,13 @@
    * This method throws an {@link InvalidDeltaException} when the delta in the {@link DataInput}
    * cannot be applied to the object. GemFire automatically handles an {@link InvalidDeltaException}
    * by reattempting the update by sending the full application object.
-   *
    */
   void fromDelta(DataInput in) throws IOException, InvalidDeltaException;
+
+  /**
+   * Allows Delta implementations to ensure bucket sizes are recalculated after delta is applied

Review comment:
       Since this method is part of the external API improve these javadocs.
   1. Add a @since 1.14
   2. Explain what geode does for delta size by default (it is calculated once when the entry is first created and any changes in actual size will not be noticed by geode. This can cause the statistics that report memory use to not be accurate), why this default was chosen (to optimize the use case in which the actual size of the object implementing Delta does not change), how to override this default behavior (implement this method in your class that implements Delta and have it return true), and explain why you would want to do that (if your Delta causes the actual size of the instance implementing Delta to change). It would also be good to point out that changing this default behavior to recalculate may hurt performance.




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