STORM-2204 Adding caching capabilities in HBaseLookupBolt by ambud · Pull Request #1783 · apache/storm

ambud · 2016-11-17T03:58:54Z

https://issues.apache.org/jira/browse/STORM-2204

revans2

I like the idea. I realize that everything seems to revolve around the guava cache, although there are alternatives to it, and I would prefer that we use one of the alternatives that has less issues with backwards compatibility.

revans2 · 2016-11-21T16:51:50Z

external/storm-hbase/pom.xml

@@ -1,92 +1,92 @@
 <?xml version="1.0" encoding="UTF-8"?>


Could you please revert the spacing back to how it was before? Almost every single line changed for what appears to be no reason.

That's happened because of my IDE. I would recommend adding the POM-Sort plugin to make sure this is auto fixed when the build runs.

I took a crack at that a few months ago: https://github.com/srotya/srotya-parent/blob/master/pom.xml

revans2 · 2016-11-21T16:55:09Z

external/storm-hbase/pom.xml

-        <groupId>org.apache.storm</groupId>
-        <version>2.0.0-SNAPSHOT</version>
-        <relativePath>../../pom.xml</relativePath>
-    </parent>


Why remove the parent? This allows us to keep a lot of dependency versions in sync so we have more confidence that there are not version conflicts when someone has a topology that includes multiple different sub-pieces

Parent is still there; I believe that is formatting from the IDE causing this.

revans2 · 2016-11-21T16:56:05Z

external/storm-hbase/pom.xml


-    <artifactId>storm-hbase</artifactId>
+	<properties>
+		<hbase.version>1.1.0</hbase.version>


The version of hbase should be coming from the parent pom, not being set here. Like I said in the previous comment, we want the parent pom to keep the dependency versions in sync between all of the sub projects.

Oh wait never mind I just saw this was brought over from the original code. You can leave it, but it would still be nice to fix.

revans2 · 2016-11-21T16:58:32Z

external/storm-hbase/pom.xml

+		<dependency>
+			<groupId>com.google.guava</groupId>
+			<artifactId>guava</artifactId>
+			<version>20.0</version>


We really should be using the version of guava in the parent pom. And guava always bites people (they really don't like to maintain compatibility) so if we can avoid using it, I would really prefer to do that.

Agree with the backwards, what are your thoughts on alternatives.

I have not really used too many alternatives, So I did a search

https://duckduckgo.com/?q=guava+cache+alternatives&t=ffab&ia=qa

Both https://cache2k.org/ and http://www.ehcache.org/ look interesting, but I have not actually used any of them

revans2 · 2016-11-21T17:00:46Z

external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/AbstractHBaseBolt.java


-    protected OutputCollector collector;
-
+    protected transient OutputCollector collector;


Why is this transient? I don't see us accessing it from multiple threads, and especially not changing it on multiple threads?

Transient is for serializability errors, I am not sure why it didn't error out in earlier when people tried to use this code.

Since Storm serializes the Bolt code for deployment this should be marked as transient.

You are right I got confused by the two. Thanks for pointing that out :)

revans2 · 2016-11-21T17:04:41Z

external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java

+	private HBaseValueMapper rowToTupleMapper;
+	private HBaseProjectionCriteria projectionCriteria;
+	private transient LoadingCache<byte[], Result> cache;
+	private transient boolean cacheEnabled;


Why are these transient? The bolt is not multi-threaded.

Transient is for serializability, not to be confused with volatile.

revans2 · 2016-11-21T17:10:11Z

external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java

@@ -40,51 +48,85 @@
 *
 */
 public class HBaseLookupBolt extends AbstractHBaseBolt {


Please don't use tab characters for indentation. We are still working on the exact style guides, but we try to keep the style that is currently in the file and it was using spaces before and I expect the style guide to include spaces instead of tabs.

Moved to spaces, if this is still a blocked; I will try to reapply the patch to the original file and then update the commits.

revans2 · 2016-11-21T17:19:02Z

external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java

+				this.collector.reportError(e);
+				this.collector.fail(tuple);
+			}
+		}


There appears to be a lot of overlap in the code here, between the cache version and the non-cache version. It would really be nice to at least combine most of this. Alternatively we might want to use inheritance in some way to avoid the branching on the cacheEnabled config at all.

… managed dependency

revans2 · 2016-11-21T19:40:15Z

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.

ben-manes · 2016-11-22T06:08:41Z

@revans2 perhaps Caffeine?

HeartSaVioR · 2016-11-22T07:49:20Z

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?

HeartSaVioR · 2016-11-22T08:06:30Z

Forgot to comment for other side. It looks great.

ben-manes · 2016-11-22T08:32:17Z

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) 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 provides an overview of the algorithms that I use.

vesense · 2016-11-22T13:16:16Z

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.

revans2 · 2016-11-22T14:28:25Z

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.

vesense · 2016-11-22T15:02:25Z

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

revans2 · 2016-11-22T15:17:32Z

@vesense it is off by default so it would be enabled/tuned on a per topology basis.

ambud · 2016-11-22T17:44:19Z

@revans2 @vesense @ben-manes Thanks for the valuable feedback, I am looking into integration with Caffeine. Should have a new commit soon.

@revans2 Indeed the intention is to have the cache tuned based on topology. The ideal use case is where caching actually makes sense and certain keys may have a higher hit frequency then others.

Ideally this bolt should be used with FieldGrouping when caching is enabled so cache misses minimal. Something to be kept in mind is distribution of keys (executor balancing).

vesense · 2016-11-23T02:46:43Z

+1

vesense · 2016-11-23T02:53:37Z

external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java

+                  @Override
+                  public Result load(byte[] rowKey) throws Exception {
+                     Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+                     LOG.debug("Cache miss for key:"+new String(rowKey));


@ambud
It would be better to use Cache miss for key: {} instead of +. After fixing this, you can squash commits into one.

@ambud
Could you address @vesense comment?
It makes unnecessary creation of String object for non-debug log level.

HeartSaVioR · 2016-11-23T02:56:05Z

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?

ben-manes · 2016-11-23T03:02:30Z

Nope. Sorry, since your compilation target is 1.8 I hadn't thought you'd need that.

ambud · 2016-11-23T03:07:38Z

So should I revert back my changes; seems like the build is currently failing; additional tests pushed by upstream?

HeartSaVioR · 2016-11-23T23:32:21Z

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

revans2 · 2016-11-28T16:10:07Z

@HeartSaVioR I also am find with Caffeine and even Caffeine on 1.x (assuming it is off by default)

revans2 · 2016-11-28T16:11:05Z

+1 the code looks fine to me.

HeartSaVioR · 2016-11-30T15:55:43Z

@ambud
The code looks good except what @vesense commented.

Two things more to address:

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

ben-manes · 2016-11-30T17:02:42Z

Should fail, e.g. Class version error.

ptgoetz · 2016-11-30T17:37:25Z

+1, but I agree with @HeartSaVioR that the new cache configuration options need to be documented before this is merged.

Adding configuration docs to README

ambud · 2016-11-30T18:12:08Z

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

HeartSaVioR · 2016-12-01T04:20:32Z

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

ambud · 2016-12-01T05:32:44Z

Caffeine is JDK 8 only so won't work for 1.x, since JDK 7 compilation will be tested.

HeartSaVioR · 2016-12-01T05:38:33Z

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

ambud · 2016-12-01T05:44:35Z

Sure thing, I will add the Guava patch code originally added for this to 1.x and 0.10.x branches

vesense · 2016-12-01T09:38:32Z

+1 for merging to master after updating docs/storm-hbase.md. external/storm-hbase/README.md has been documented.

revans2 · 2016-12-01T14:19:17Z

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.

HeartSaVioR · 2016-12-01T14:55:37Z

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.

ambud · 2016-12-01T20:24:10Z

Can we merge this? @HeartSaVioR @revans2 @vesense

I will need to open another pull request for 1.x branch.

ambud · 2016-12-01T20:26:00Z

1.x branch PR #1810

ambud · 2016-12-10T00:21:29Z

Can we merge this @revans2 @HeartSaVioR @vesense ?

vesense · 2016-12-15T05:36:25Z

@ambud Sorry for the delay response. Before we merge this in, can you replace all the tab space to white space?

ambud · 2016-12-15T05:53:31Z

Done @vesense

vesense · 2016-12-15T09:12:48Z

Thanks @ambud Squashed and merged into master. And I added you as the contributor.

ambud · 2016-12-15T15:08:36Z

Thank you @vesense. Could we merge the 1.x pull request as well?

vesense · 2016-12-16T01:45:26Z

@ambud I'll wait for other committers to vote for it before merging into 1.x-branch. And this may take some time.

Adding caching capabilities on bolt side

5f048ca

ambud changed the title ~~Adding caching capabilities on bolt side~~ STORM-2204 Adding caching capabilities on bolt side Nov 17, 2016

ambud changed the title ~~STORM-2204 Adding caching capabilities on bolt side~~ STORM-2204 Adding caching capabilities in HBaseLookupBolt Nov 18, 2016

revans2 reviewed Nov 21, 2016

View reviewed changes

Refactoring code for branching when using caching. Using guava parent…

e244f0d

… managed dependency

Ambud Sharma and others added 3 commits November 22, 2016 10:10

Moving to Caffeine based caching

83d9044

Fixing formatting

dd9a054

Adding debug statements for logging and troubleshooting

bf770c1

vesense reviewed Nov 23, 2016

View reviewed changes

Adding debug logging

d3cb605

Adding configuration docs to README

Resolving conflicts

5c95bde

ambud mentioned this pull request Dec 1, 2016

STORM-2204 Adding Bolt side caching for HBase Lookup Bolt for 1.x branch #1810

Merged

Replacing tabs with spaces

385e70e

asfgit closed this in 610d58d Dec 15, 2016


		protected OutputCollector collector;

		protected transient OutputCollector collector;

Conversation

ambud commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

revans2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revans2 commented Nov 21, 2016

Uh oh!

ben-manes commented Nov 22, 2016

Uh oh!

HeartSaVioR commented Nov 22, 2016

Uh oh!

HeartSaVioR commented Nov 22, 2016

Uh oh!

ben-manes commented Nov 22, 2016

Uh oh!

vesense commented Nov 22, 2016

Uh oh!

revans2 commented Nov 22, 2016

Uh oh!

vesense commented Nov 22, 2016

Uh oh!

revans2 commented Nov 22, 2016

Uh oh!

ambud commented Nov 22, 2016

Uh oh!

vesense commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Nov 23, 2016

Uh oh!

ben-manes commented Nov 23, 2016

Uh oh!

ambud commented Nov 23, 2016

Uh oh!

HeartSaVioR commented Nov 23, 2016

Uh oh!

revans2 commented Nov 28, 2016

Uh oh!

revans2 commented Nov 28, 2016

Uh oh!

HeartSaVioR commented Nov 30, 2016

ambud commented Nov 17, 2016 •

edited

Loading

ambud commented Dec 15, 2016 •

edited

Loading