WIP: Fix for FLUO-938 #941

kennethmcfarland · 2017-10-12T23:06:10Z

No description provided.

keith-turner · 2017-10-13T14:21:51Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+
+  // Note the size of the cache is in megabytes
+  public static final String TX_INFO_CACHE_SIZE = FLUO_IMPL_PREFIX + ".tx.failed.cache.size.mb";
+  /* TODO: What is a reasonable default size? Currently CacheBuilder does not specify a size in TxInfoCache.java */


It indirectly specifies a size by specifying at weight and weigher. A weigher could return anything, in this case it returns size. The TxStatusWeigher returns the size + 24 (there is not coment, but I assume the 24 is some estimated overhead). Currently its specifies a max weight of 10,000,000, so that would be a good default. Need to look for other config that has a size in mb and see if multiplies by 10^6 or 2^20, it would be good to be consistent.

I did some digging and found three precedents. In FluoConfigurationImpl we have a commit memory which has a unit of bytes. In FluoYarnConfig we have fluo.yarn.worker.max.memory.mb these end up being passed to Twill and it seems that Twill has a SizeUnit class. However the Twill SizeUnit class does not document if MEGA is 10^6 or 2^20. Based on past experience I think it may 2^20, but not sure. I would say multiply by 2^20 for now.

keith-turner · 2017-10-13T14:39:29Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+   * 
+   * @param conf
+   * @return The shelf life of entries before they will be evicted if not accessed
+   * TODO: What timeunit would make most sense? Currently TxCacheInfo uses TimeUnit.MINUTES


Thats a very good question. I looked at some time props in FluoConfiguration and the timeunits are mostly well documented because of your previous change. Need to document the timeunit for zookeeper timeout, this could be a new help wanted issue, would you like to open that?

I did find one prop with a pattern I really liked in FluoConfiguration

public FluoConfiguration setTransactionRollbackTime(long time, TimeUnit tu) { return setPositiveLong(TRANSACTION_ROLLBACK_TIME_PROP, tu.toMillis(time)); }

I like two things about this pattern :

In the java API it uses timeunit, so that makes the code very clear and explicit.

Internally it converts everything to milliseconds and the prop name ends with ms.

Something that transaction rollback time didn't do that would have been nice would be to accept a timeunit on the getter. For cache timeout, this could be something like long getTxInfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) and it will return the timeout in the specified unit.

This makes me think it would be nice to have a SizeUnit, I am not sure if this is a good idea or not. May be worthwhile opening an issue to discuss. Could make config API nicer.

keith-turner · 2017-10-17T00:29:07Z

@kpm1985 sorry I have not been able to look at your latest changes sooner. I plan on looking at them tomorrow afternoon.

keith-turner · 2017-10-17T19:43:23Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+
+  /**
+   * Gets the time before stale entries in the cache are evicted based on age.
+   * This method returns a long representing the time in milliseconds.


Can replace time in milliseconds with time in passed in unit.

keith-turner · 2017-10-17T19:43:31Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+
+  public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) {
+    int minutes = getTxInfoCacheTimeout(conf);
+    return tu.toMillis((long) minutes);


I think this should be return tu.convert(minutes, TimeUnit.MINUTES). This will convert minutes to the passed in time unit.

keith-turner · 2017-10-17T19:46:06Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+   * @param conf FluoConfiguration
+   * @return the time in minutes
+   */
+  public static int getTxInfoCacheTimeout(FluoConfiguration conf) {


This method could be dropped and its functionality inlined in the method getTxIfoCacheTimeout(FluoConfiguration, TimeUnit)

keith-turner · 2017-10-17T19:47:49Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+  }
+
+  public static final String TX_INFO_CACHE_TIMEOUT =
+      FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime";


Could add a suffix of minutes like .tx.failed.cache.expireTime.minutes

kennethmcfarland · 2017-10-17T20:45:36Z

Thank you for this review @keith-turner I'll be addressing these very soon!

…

On Oct 17, 2017 12:48 PM, "Keith Turner" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + /** + * Gets the time before stale cache entries are evicted + * @param conf FluoConfiguration + * @return the time in minutes + */ + public static int getTxInfoCacheTimeout(FluoConfiguration conf) { + int timeout = conf.getInt(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT); + if (timeout <= 0) { + throw new IllegalArgumentException("Timout must positive for " + TX_INFO_CACHE_TIMEOUT); + } + return timeout; + } + + /** + * Gets the time before stale entries in the cache are evicted based on age. + * This method returns a long representing the time in milliseconds. Can replace time in milliseconds with time in passed in unit. ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + if (timeout <= 0) { + throw new IllegalArgumentException("Timout must positive for " + TX_INFO_CACHE_TIMEOUT); + } + return timeout; + } + + /** + * Gets the time before stale entries in the cache are evicted based on age. + * This method returns a long representing the time in milliseconds. + * + ***@***.*** 1.2.0 + */ + + public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) { + int minutes = getTxInfoCacheTimeout(conf); + return tu.toMillis((long) minutes); I think this should be return tu.convert(minutes, TimeUnit.MINUTES). This will convert minutes to the passed in time unit. ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + throw new IllegalArgumentException("Cache size must be positive for " + TX_INFO_CACHE_SIZE); + } + return size; + } + + public static final String TX_INFO_CACHE_TIMEOUT = + FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime"; + /* Note: this default is in minutes */ + public static final int TX_INFO_CACHE_TIMEOUT_DEFAULT = 24 * 60; + + /** + * Gets the time before stale cache entries are evicted + * @param conf FluoConfiguration + * @return the time in minutes + */ + public static int getTxInfoCacheTimeout(FluoConfiguration conf) { This method could be dropped and its functionality inlined in the method getTxIfoCacheTimeout(FluoConfiguration, TimeUnit) ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + * @param conf The FluoConfiguration. + * @return The size of the cache value from the property value ***@***.*** #TX_INFO_CACHE_SIZE} + * if it is set, else the value of the default value ***@***.*** #TX_INFO_CACHE_SIZE_DEFAULT} + * @SInCE 1.2.0 + */ + + public static long getTxInfoCacheSize(FluoConfiguration conf) { + long size = conf.getLong(TX_INFO_CACHE_SIZE, TX_INFO_CACHE_SIZE_DEFAULT); + if (size <= 0) { + throw new IllegalArgumentException("Cache size must be positive for " + TX_INFO_CACHE_SIZE); + } + return size; + } + + public static final String TX_INFO_CACHE_TIMEOUT = + FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime"; Could add a suffix of minutes like .tx.failed.cache.expireTime.minutes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#941 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Acg-LFTB5zfcLZncR9U3BSJTiSslU8OWks5stQSDgaJpZM4P3wLr> .

keith-turner

I think this code looks good and is ready to be called by TxInfoCache constructor. Changing the internal units from minutes to milliseconds could be done as a follow on issue or in this PR.

keith-turner · 2017-10-18T19:14:25Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+  }
+
+  public static final String TX_INFO_CACHE_TIMEOUT =
+      FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime.minutes";


I think minutes makes sense for this property. However I am thinking we should change the unit to milliseconds for consistency. Some other time based configs may need more granularity. Going forward, I think it would be nice if all new time based configs added had the same unit. This results in a less confusing experience when considering the config as a whole.

We can use a suffix of .ms for the property name.

keith-turner · 2017-10-18T19:16:17Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+  public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) {
+    long minutes = (long) conf.getInt(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT);
+    if (minutes <= 0) {
+      throw new IllegalArgumentException("Timout must positive for " + TX_INFO_CACHE_TIMEOUT);


Replace Timout must positive with Timeout must be positive.

keith-turner · 2017-10-18T19:16:49Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+   */
+
+  public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) {
+    long minutes = (long) conf.getInt(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT);


Does conf have a getLong() method?

keith-turner · 2017-10-18T19:17:48Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+  public static final long TX_INFO_CACHE_SIZE_DEFAULT = 10000000;
+
+  /** 
+   * Gets the value of the property {@value #TX_INFO_CACHE_SIZE} if set,


I think this can be dropped as it is well covered in the @return docs.

keith-turner · 2017-10-18T19:18:47Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+   * @return The size of the cache value from the property value {@value #TX_INFO_CACHE_SIZE}
+   *     if it is set, else the value of the default value {@value #TX_INFO_CACHE_SIZE_DEFAULT}
+   *     
+   * @since 1.2.0


Adding @since here is optional as this is not public API.

@SInCE

…es to be consistent, use conf.getLong which no longer needs boxing or casting, remove @SInCE tags for private class. -Kenneth McFarland

kennethmcfarland · 2017-10-18T19:43:46Z

I changed the data type for the default property to simply be a long instead of an int. Same functionality, no more boxing or casting and can use conf.getLong().

keith-turner · 2017-10-18T19:56:34Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+   */
+
+  public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) {
+    long minutes = conf.getLong(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT);


should change this var name from minutes to something like millis

keith-turner · 2017-10-18T19:57:01Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+    if (minutes <= 0) {
+      throw new IllegalArgumentException("Timeout must positive for " + TX_INFO_CACHE_TIMEOUT);
+    }
+    return tu.convert(minutes, TimeUnit.MINUTES);


Should change the timeunit from MINUTES to MILLISECONDS

keith-turner · 2017-10-18T19:57:26Z

modules/core/src/main/java/org/apache/fluo/core/impl/FluoConfigurationImpl.java

+
+  public static final String TX_INFO_CACHE_TIMEOUT =
+      FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime.ms";
+  public static final long TX_INFO_CACHE_TIMEOUT_DEFAULT = 24 * 60;


Should set this to 24 * 60 * 1000

…y necessary changes

kennethmcfarland · 2017-10-18T20:45:39Z

@keith-turner I get what the intent was. Hopefully QED

…

On Oct 18, 2017 12:59 PM, "Keith Turner" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + + public static final String TX_INFO_CACHE_TIMEOUT = + FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime.ms"; + public static final long TX_INFO_CACHE_TIMEOUT_DEFAULT = 24 * 60; + + /** + * Gets the time before stale entries in the cache are evicted based on age. + * This method returns a long representing the time converted from the + * TimeUnit passed in. + * + * @param conf The FluoConfiguration + * @param tu The TimeUnit desired to represent the cache timeout + */ + + public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) { + long minutes = conf.getLong(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT); should change this var name from minutes to something like millis ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + + /** + * Gets the time before stale entries in the cache are evicted based on age. + * This method returns a long representing the time converted from the + * TimeUnit passed in. + * + * @param conf The FluoConfiguration + * @param tu The TimeUnit desired to represent the cache timeout + */ + + public static long getTxIfoCacheTimeout(FluoConfiguration conf, TimeUnit tu) { + long minutes = conf.getLong(TX_INFO_CACHE_TIMEOUT, TX_INFO_CACHE_TIMEOUT_DEFAULT); + if (minutes <= 0) { + throw new IllegalArgumentException("Timeout must positive for " + TX_INFO_CACHE_TIMEOUT); + } + return tu.convert(minutes, TimeUnit.MINUTES); Should change the timeunit from MINUTES to MILLISECONDS ------------------------------ In modules/core/src/main/java/org/apache/fluo/core/impl/ FluoConfigurationImpl.java <#941 (comment)>: > + * @param conf The FluoConfiguration + * @return The size of the cache value from the property value ***@***.*** #TX_INFO_CACHE_SIZE} + * if it is set, else the value of the default value ***@***.*** #TX_INFO_CACHE_SIZE_DEFAULT} + */ + + public static long getTxInfoCacheSize(FluoConfiguration conf) { + long size = conf.getLong(TX_INFO_CACHE_SIZE, TX_INFO_CACHE_SIZE_DEFAULT); + if (size <= 0) { + throw new IllegalArgumentException("Cache size must be positive for " + TX_INFO_CACHE_SIZE); + } + return size; + } + + public static final String TX_INFO_CACHE_TIMEOUT = + FLUO_IMPL_PREFIX + ".tx.failed.cache.expireTime.ms"; + public static final long TX_INFO_CACHE_TIMEOUT_DEFAULT = 24 * 60; Should set this to 24 * 60 * 1000 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#941 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Acg-LNG_p73nckYbGVfMegbIA5aI2wm-ks5stliLgaJpZM4P3wLr> .

keith-turner · 2017-10-18T20:58:29Z

@kpm1985 these changes look great. After make the cache code call these new config methods, this will be ready to merge.

kennethmcfarland · 2017-10-18T22:24:21Z

@keith-turner Thank you, I've modified the constructor to take the new methods

kennethmcfarland · 2017-10-19T00:06:11Z

@keith-turner thank you, I made a few changes to the constructor

…

On Oct 18, 2017 1:58 PM, "Keith Turner" ***@***.***> wrote: @kpm1985 <https://github.com/kpm1985> these changes look great. After make the cache code call these new config methods, this will be ready to merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#941 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Acg-LC02uc0dgkFb0-ttv2b88tKAzlBiks5stmZ1gaJpZM4P3wLr> .

keith-turner

This looks good. Thanks again @kpm1985

Kenneth added 2 commits October 12, 2017 16:04

WIP: Fix for FLUO-938

33bf680

WIP: Additional additions, add javadoc, add TODO items

08a2d8b

keith-turner reviewed Oct 13, 2017

View reviewed changes

Kenneth added 3 commits October 13, 2017 19:47

WIP - First Round Revisions

139bbdb

Fix Javadoc errors

ccbc59d

Fix checkstyle violations

8d3b270

keith-turner reviewed Oct 17, 2017

View reviewed changes

Second round revisions

a1e19bb

keith-turner reviewed Oct 18, 2017

View reviewed changes

FLUO-938 Third round revisions: Fix typo, use long to represent minut…

412add4

…es to be consistent, use conf.getLong which no longer needs boxing or casting, remove @SInCE tags for private class. -Kenneth McFarland

Fix redundant Javadoc

712aeea

keith-turner reviewed Oct 18, 2017

View reviewed changes

Increase granularit of default cache timeout to milliseconds and appl…

8c98224

…y necessary changes

Modify constructor to use new methods in FluoConfigurationImpl

ae4bdcf

keith-turner approved these changes Oct 19, 2017

View reviewed changes

asfgit pushed a commit that referenced this pull request Oct 19, 2017

fixes #938 made tx info cache configurable (#941)

650f044

keith-turner closed this Oct 19, 2017

kennethmcfarland deleted the FLUO-938 branch October 19, 2017 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix for FLUO-938 #941

WIP: Fix for FLUO-938 #941

kennethmcfarland commented Oct 12, 2017

keith-turner Oct 13, 2017

keith-turner Oct 13, 2017

keith-turner Oct 13, 2017

keith-turner commented Oct 17, 2017

keith-turner Oct 17, 2017

keith-turner Oct 17, 2017

keith-turner Oct 17, 2017

keith-turner Oct 17, 2017

kennethmcfarland commented Oct 17, 2017 via email

keith-turner left a comment

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

kennethmcfarland commented Oct 18, 2017

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

keith-turner Oct 18, 2017

kennethmcfarland commented Oct 18, 2017 via email

keith-turner commented Oct 18, 2017

kennethmcfarland commented Oct 18, 2017

kennethmcfarland commented Oct 19, 2017 via email

keith-turner left a comment

WIP: Fix for FLUO-938 #941

WIP: Fix for FLUO-938 #941

Conversation

kennethmcfarland commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennethmcfarland commented Oct 17, 2017 via email

keith-turner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennethmcfarland commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennethmcfarland commented Oct 18, 2017 via email

keith-turner commented Oct 18, 2017

kennethmcfarland commented Oct 18, 2017

kennethmcfarland commented Oct 19, 2017 via email

keith-turner left a comment

Choose a reason for hiding this comment