[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 @ambud I'll wait for other committers to vote for it before merging into 1.x-branch. And this may take some time. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Thank you @vesense. Could we merge the 1.x pull request as well? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 Thanks @ambud Squashed and merged into master. And I added you as the contributor. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Done --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 @ambud Sorry for the delay response. Before we merge this in, can you replace all the tab space to white space? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Can we merge this @revans2 @HeartSaVioR @vesense ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 1.x branch PR https://github.com/apache/storm/pull/1810 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Can we merge this? @HeartSaVioR @revans2 @vesense I will need to open another pull request for 1.x branch. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 OK. Since we have other modules which depends on Guava, it might be better to have a rule and apply all of them. +1 to shade Guava on external modules if possible. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 I guess guava is OK for 1.x. I would prefer to see it shaded if we do go with Guava, but I am only a -0 if it is not shaded. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 +1 for merging to master after updating `docs/storm-hbase.md`. `external/storm-hbase/README.md` has been documented. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Sure thing, I will add the Guava patch code originally added for this to 1.x and 0.10.x branches --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 @ambud Yeah if we add unit test for the feature, unit test will fail on JDK 7 which will be always failing on Travis CI. Nice catch. Could you address this to use Guava for 1.x? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Caffeine is JDK 8 only so won't work for 1.x, since JDK 7 compilation will be tested. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 @ambud I'm OK to use Guava for 1.x. IMHO it would be better to provide complete set of features for guaranteed environment (JDK version) instead of leaving 'warn' to documentation. And some other external modules also use Guava, too. @revans2 Are you OK to use Guava only for 1.x? Or you would like to keep using Caffeine for also 1.x and document it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 Added documentation to readme file. Fixed the debug logging by using isDebugEnabled checks Are we going to use Guava caching implementation for 1.x? @HeartSaVioR @revans2 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1783 +1, but I agree with @HeartSaVioR that the new cache configuration options need to be documented before this is merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ben-manes commented on the issue: https://github.com/apache/storm/pull/1783 Should fail, e.g. Class version error. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 @ambud The code looks good except what @vesense commented. Two things more to address: 1. It would be better to document new configurations. Without documentation, end-users have no idea about added feature. `external/storm-hbase/README.md` and `docs/storm-hbase.md`. 2. The code already uses JDK 8 API (Map.getOrDefault()), so can't get it as it is for 1.x. Could you provide a new PR for 1.x branch? It would be also great if you can test it (with Caffeine) on JRE7 (expected to not work but we can document the precondition for JRE version) and JRE8 (expected to work). cc. @ben-manes Is my expectation right? Thanks in advance! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 +1 the code looks fine to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 @HeartSaVioR I also am find with Caffeine and even Caffeine on 1.x (assuming it is off by default) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 @revans2 What do you think about this? I'm in favor of adopting Caffeine, and I'm even OK to use Caffeine to master and Guava to 1.x. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ambud commented on the issue: https://github.com/apache/storm/pull/1783 So should I revert back my changes; seems like the build is currently failing; additional tests pushed by upstream? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ben-manes commented on the issue: https://github.com/apache/storm/pull/1783 Nope. Sorry, since your compilation target is 1.8 I hadn't thought you'd need that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 One thing to consider is that Caffeine seems to require Java 8, which means that this patch can't be shipped to 1.x version line. Do we want to keep using Guava for 1.x branch? @ben-manes Is there any versions for Caffeine which works with Java 7? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 @vesense it is off by default so it would be enabled/tuned on a per topology basis. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 @revans2 Yes, we can find a balance between high rate of cache hits and full GCs. I'm OK for adding a built-in cache if we set parameters carefully. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 Caffeine sounds like a great alternative to Guava and also seems to address some GC concerns. @vesense I agree storing any large amount of data on heap will impact GC, we do that all the time with all of the queues that we have. I think for the most part as long as GC is tuned properly for the topology, we don't get a lot of full GCs causing promotion in the cache, and TTL in the cache is not too long it will be fine. If we do run into serious GC issues we can then look at off heap cacheing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user vesense commented on the issue: https://github.com/apache/storm/pull/1783 Overall looks good to me. My major concern is that on-heap caches(like Guava cache, Ehcache) might cause bad GC situations. I didn't use Caffeine in the past, maybe is a candidate. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ben-manes commented on the issue: https://github.com/apache/storm/pull/1783 I was a co-author of Guava's cache, too. Guava had originally considered soft references an ideal caching scheme, since they offer great concurrency and GC is for memory management. That evolved from `ReferenceMap` to `MapMaker` to optimize space, especially in regards to computations (no need for a `Future` wrapper). Unfortunately soft references result in awful performance outside of a micro-benchmark due to causing full GCs. In parallel, I had been experimenting with approaches for a concurrent LRU cache ([CLHM](https://github.com/ben-manes/concurrentlinkedhashmap)) and when joining Google helped to retrofitted its ideas onto Guava. There was a lot of good that came out of that, but I left before working on optimizing for performance. Java 8 provided an excuse to start from scratch. Caffeine is much faster and packs in even more features. I also spent time exploring eviction policies, which led to co-authoring a paper on a new technique called TinyLFU. That has a near optimal hit rate, low memory footprint, and amortized O(1) overhead. This is done by tracking frequency in a popularity sketch. The same concurrency model in CLHM and Guava is used (inspired by a write-ahead log), which allows for concurrent O(1) reads and writes. The [HighScalability article](http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html) provides an overview of the algorithms that I use. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 Forgot to comment for other side. It looks great. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1783 I agree it's still better to not relying on Guava since storm-core is shading Guava but external modules are not. Btw, I didn't use other cache modules too, but ehcache seems to be well-known and widely-used cache library, and Caffeine seems to be a drop-in replacement for Guava cache. @ben-manes I guess you're the author of Caffeine. Could you introduce Caffeine? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user ben-manes commented on the issue: https://github.com/apache/storm/pull/1783 @revans2 perhaps [Caffeine](https://github.com/ben-manes/caffeine)? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/1783 Overall it looks fine to me now. I am still a bit concerned about guava as a dependency, but beyond that it seems like a great addition to the bolt. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
