You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by ambud <gi...@git.apache.org> on 2016/11/17 03:58:57 UTC
[GitHub] storm pull request #1783: Adding caching capabilities on bolt side
GitHub user ambud opened a pull request:
https://github.com/apache/storm/pull/1783
Adding caching capabilities on bolt side
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ambud/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1783.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1783
----
commit 5f048ca76ef60bb272c58b24ccddc5674892fb4f
Author: Ambud Sharma <am...@hw11015.hsd1.ca.comcast.net>
Date: 2016-11-17T03:09:22Z
Adding caching capabilities on bolt side
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r90260820
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,81 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
- private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private static final long serialVersionUID = 1L;
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private HBaseValueMapper rowToTupleMapper;
+ private HBaseProjectionCriteria projectionCriteria;
+ private transient LoadingCache<byte[], Result> cache;
+ private transient boolean cacheEnabled;
- private HBaseValueMapper rowToTupleMapper;
+ public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper) {
+ super(tableName, mapper);
+ Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
+ this.rowToTupleMapper = rowToTupleMapper;
+ }
- private HBaseProjectionCriteria projectionCriteria;
+ public HBaseLookupBolt withConfigKey(String configKey) {
+ this.configKey = configKey;
+ return this;
+ }
- public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper){
- super(tableName, mapper);
- Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
- this.rowToTupleMapper = rowToTupleMapper;
- }
+ public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
+ this.projectionCriteria = projectionCriteria;
+ return this;
+ }
- public HBaseLookupBolt withConfigKey(String configKey){
- this.configKey = configKey;
- return this;
- }
+ @SuppressWarnings({ "unchecked", "rawtypes" })
+ @Override
+ public void prepare(Map config, TopologyContext topologyContext, OutputCollector collector) {
+ super.prepare(config, topologyContext, collector);
+ cacheEnabled = Boolean.parseBoolean(config.getOrDefault("hbase.cache.enable", "false").toString());
+ int cacheTTL = Integer.parseInt(config.getOrDefault("hbase.cache.ttl.seconds", "300").toString());
+ int maxCacheSize = Integer.parseInt(config.getOrDefault("hbase.cache.size", "1000").toString());
+ if (cacheEnabled) {
+ cache = Caffeine.newBuilder()
+ .maximumSize(maxCacheSize).expireAfterWrite(cacheTTL, TimeUnit.SECONDS)
+ .build(new CacheLoader<byte[], Result>() {
- public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
- this.projectionCriteria = projectionCriteria;
- return this;
- }
+ @Override
+ public Result load(byte[] rowKey) throws Exception {
+ Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+ LOG.debug("Cache miss for key:"+new String(rowKey));
--- End diff --
@ambud
Could you address @vesense comment?
It makes unnecessary creation of String object for non-debug log level.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88939110
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
+ <developers>
+ <developer>
+ <id>ptgoetz</id>
+ <name>P. Taylor Goetz</name>
+ <email>ptgoetz@gmail.com</email>
+ </developer>
+ </developers>
- <artifactId>storm-hbase</artifactId>
+ <properties>
+ <hbase.version>1.1.0</hbase.version>
+ <hdfs.version>${hadoop.version}</hdfs.version>
+ </properties>
- <developers>
- <developer>
- <id>ptgoetz</id>
- <name>P. Taylor Goetz</name>
- <email>ptgoetz@gmail.com</email>
- </developer>
- </developers>
-
- <properties>
- <hbase.version>1.1.0</hbase.version>
- <hdfs.version>${hadoop.version}</hdfs.version>
- </properties>
-
- <dependencies>
- <dependency>
- <groupId>org.apache.storm</groupId>
- <artifactId>storm-core</artifactId>
- <version>${project.version}</version>
- <scope>provided</scope>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-server</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-client</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hadoop</groupId>
- <artifactId>hadoop-hdfs</artifactId>
- <version>${hdfs.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- </dependencies>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.storm</groupId>
+ <artifactId>storm-core</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-server</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-client</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-hdfs</artifactId>
+ <version>${hdfs.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>20.0</version>
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88938511
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
+ <developers>
+ <developer>
+ <id>ptgoetz</id>
+ <name>P. Taylor Goetz</name>
+ <email>ptgoetz@gmail.com</email>
+ </developer>
+ </developers>
- <artifactId>storm-hbase</artifactId>
+ <properties>
+ <hbase.version>1.1.0</hbase.version>
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88964361
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
+ <developers>
+ <developer>
+ <id>ptgoetz</id>
+ <name>P. Taylor Goetz</name>
+ <email>ptgoetz@gmail.com</email>
+ </developer>
+ </developers>
- <artifactId>storm-hbase</artifactId>
+ <properties>
+ <hbase.version>1.1.0</hbase.version>
+ <hdfs.version>${hadoop.version}</hdfs.version>
+ </properties>
- <developers>
- <developer>
- <id>ptgoetz</id>
- <name>P. Taylor Goetz</name>
- <email>ptgoetz@gmail.com</email>
- </developer>
- </developers>
-
- <properties>
- <hbase.version>1.1.0</hbase.version>
- <hdfs.version>${hadoop.version}</hdfs.version>
- </properties>
-
- <dependencies>
- <dependency>
- <groupId>org.apache.storm</groupId>
- <artifactId>storm-core</artifactId>
- <version>${project.version}</version>
- <scope>provided</scope>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-server</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-client</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hadoop</groupId>
- <artifactId>hadoop-hdfs</artifactId>
- <version>${hdfs.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- </dependencies>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.storm</groupId>
+ <artifactId>storm-core</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-server</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-client</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-hdfs</artifactId>
+ <version>${hdfs.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>20.0</version>
--- End diff --
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
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r89250230
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,81 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
- private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private static final long serialVersionUID = 1L;
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private HBaseValueMapper rowToTupleMapper;
+ private HBaseProjectionCriteria projectionCriteria;
+ private transient LoadingCache<byte[], Result> cache;
+ private transient boolean cacheEnabled;
- private HBaseValueMapper rowToTupleMapper;
+ public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper) {
+ super(tableName, mapper);
+ Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
+ this.rowToTupleMapper = rowToTupleMapper;
+ }
- private HBaseProjectionCriteria projectionCriteria;
+ public HBaseLookupBolt withConfigKey(String configKey) {
+ this.configKey = configKey;
+ return this;
+ }
- public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper){
- super(tableName, mapper);
- Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
- this.rowToTupleMapper = rowToTupleMapper;
- }
+ public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
+ this.projectionCriteria = projectionCriteria;
+ return this;
+ }
- public HBaseLookupBolt withConfigKey(String configKey){
- this.configKey = configKey;
- return this;
- }
+ @SuppressWarnings({ "unchecked", "rawtypes" })
+ @Override
+ public void prepare(Map config, TopologyContext topologyContext, OutputCollector collector) {
+ super.prepare(config, topologyContext, collector);
+ cacheEnabled = Boolean.parseBoolean(config.getOrDefault("hbase.cache.enable", "false").toString());
+ int cacheTTL = Integer.parseInt(config.getOrDefault("hbase.cache.ttl.seconds", "300").toString());
+ int maxCacheSize = Integer.parseInt(config.getOrDefault("hbase.cache.size", "1000").toString());
+ if (cacheEnabled) {
+ cache = Caffeine.newBuilder()
+ .maximumSize(maxCacheSize).expireAfterWrite(cacheTTL, TimeUnit.SECONDS)
+ .build(new CacheLoader<byte[], Result>() {
- public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
- this.projectionCriteria = projectionCriteria;
- return this;
- }
+ @Override
+ public Result load(byte[] rowKey) throws Exception {
+ Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+ LOG.debug("Cache miss for key:"+new String(rowKey));
--- End diff --
@ambud
It would be better to use `Cache miss for key: {}` instead of `+`. After fixing this, you can squash commits into one.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ben-manes <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ben-manes <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ben-manes <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88946224
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,85 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
- private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private HBaseValueMapper rowToTupleMapper;
+ private HBaseProjectionCriteria projectionCriteria;
+ private transient LoadingCache<byte[], Result> cache;
+ private transient boolean cacheEnabled;
--- End diff --
Transient is for serializability, not to be confused with volatile.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88937595
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
--- End diff --
Could you please revert the spacing back to how it was before? Almost every single line changed for what appears to be no reason.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on the issue:
https://github.com/apache/storm/pull/1783
@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).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88943466
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,85 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
- private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private HBaseValueMapper rowToTupleMapper;
+ private HBaseProjectionCriteria projectionCriteria;
+ private transient LoadingCache<byte[], Result> cache;
+ private transient boolean cacheEnabled;
- private HBaseValueMapper rowToTupleMapper;
+ public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper) {
+ super(tableName, mapper);
+ Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
+ this.rowToTupleMapper = rowToTupleMapper;
+ }
- private HBaseProjectionCriteria projectionCriteria;
+ public HBaseLookupBolt withConfigKey(String configKey) {
+ this.configKey = configKey;
+ return this;
+ }
- public HBaseLookupBolt(String tableName, HBaseMapper mapper, HBaseValueMapper rowToTupleMapper){
- super(tableName, mapper);
- Validate.notNull(rowToTupleMapper, "rowToTupleMapper can not be null");
- this.rowToTupleMapper = rowToTupleMapper;
- }
+ public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
+ this.projectionCriteria = projectionCriteria;
+ return this;
+ }
- public HBaseLookupBolt withConfigKey(String configKey){
- this.configKey = configKey;
- return this;
- }
+ @SuppressWarnings({ "unchecked", "rawtypes" })
+ @Override
+ public void prepare(Map config, TopologyContext topologyContext, OutputCollector collector) {
+ super.prepare(config, topologyContext, collector);
+ cacheEnabled = Boolean.parseBoolean(config.getOrDefault("hbase.cache.enable", "false").toString());
+ int cacheTTL = Integer.parseInt(config.getOrDefault("hbase.cache.ttl.seconds", "300").toString());
+ int maxCacheSize = Integer.parseInt(config.getOrDefault("hbase.cache.size", "1000").toString());
+ if (cacheEnabled) {
+ cache = CacheBuilder.newBuilder().maximumSize(maxCacheSize).expireAfterWrite(cacheTTL, TimeUnit.SECONDS)
+ .build(new CacheLoader<byte[], Result>() {
- public HBaseLookupBolt withProjectionCriteria(HBaseProjectionCriteria projectionCriteria) {
- this.projectionCriteria = projectionCriteria;
- return this;
- }
+ @Override
+ public Result load(byte[] rowKey) throws Exception {
+ Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+ return hBaseClient.batchGet(Lists.newArrayList(get))[0];
+ }
- @Override
- public void execute(Tuple tuple) {
- if (TupleUtils.isTick(tuple)) {
- collector.ack(tuple);
- return;
- }
- byte[] rowKey = this.mapper.rowKey(tuple);
- Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+ });
+ }
+ }
- try {
- Result result = hBaseClient.batchGet(Lists.newArrayList(get))[0];
- for(Values values : rowToTupleMapper.toValues(tuple, result)) {
- this.collector.emit(tuple, values);
- }
- this.collector.ack(tuple);
- } catch (Exception e) {
- this.collector.reportError(e);
- this.collector.fail(tuple);
- }
- }
+ @Override
+ public void execute(Tuple tuple) {
+ if (TupleUtils.isTick(tuple)) {
+ collector.ack(tuple);
+ return;
+ }
+ byte[] rowKey = this.mapper.rowKey(tuple);
+ if (cacheEnabled) {
+ try {
+ Result result = cache.get(rowKey);
+ for (Values values : rowToTupleMapper.toValues(tuple, result)) {
+ this.collector.emit(tuple, values);
+ }
+ this.collector.ack(tuple);
+ } catch (Exception e) {
+ this.collector.reportError(e);
+ this.collector.fail(tuple);
+ }
+ } else {
+ Get get = hBaseClient.constructGetRequests(rowKey, projectionCriteria);
+ try {
+ Result result = hBaseClient.batchGet(Lists.newArrayList(get))[0];
+ for (Values values : rowToTupleMapper.toValues(tuple, result)) {
+ this.collector.emit(tuple, values);
+ }
+ this.collector.ack(tuple);
+ } catch (Exception e) {
+ this.collector.reportError(e);
+ this.collector.fail(tuple);
+ }
+ }
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88945432
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
--- End diff --
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
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88949169
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
+ <developers>
+ <developer>
+ <id>ptgoetz</id>
+ <name>P. Taylor Goetz</name>
+ <email>ptgoetz@gmail.com</email>
+ </developer>
+ </developers>
- <artifactId>storm-hbase</artifactId>
+ <properties>
+ <hbase.version>1.1.0</hbase.version>
+ <hdfs.version>${hadoop.version}</hdfs.version>
+ </properties>
- <developers>
- <developer>
- <id>ptgoetz</id>
- <name>P. Taylor Goetz</name>
- <email>ptgoetz@gmail.com</email>
- </developer>
- </developers>
-
- <properties>
- <hbase.version>1.1.0</hbase.version>
- <hdfs.version>${hadoop.version}</hdfs.version>
- </properties>
-
- <dependencies>
- <dependency>
- <groupId>org.apache.storm</groupId>
- <artifactId>storm-core</artifactId>
- <version>${project.version}</version>
- <scope>provided</scope>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-server</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hbase</groupId>
- <artifactId>hbase-client</artifactId>
- <version>${hbase.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- <exclusion>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- <dependency>
- <groupId>org.apache.hadoop</groupId>
- <artifactId>hadoop-hdfs</artifactId>
- <version>${hdfs.version}</version>
- <exclusions>
- <exclusion>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </exclusion>
- </exclusions>
- </dependency>
- </dependencies>
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.storm</groupId>
+ <artifactId>storm-core</artifactId>
+ <version>${project.version}</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-server</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hbase</groupId>
+ <artifactId>hbase-client</artifactId>
+ <version>${hbase.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-hdfs</artifactId>
+ <version>${hdfs.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>20.0</version>
--- End diff --
Agree with the backwards, what are your thoughts on alternatives.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88938308
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
--- End diff --
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
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ben-manes <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88941608
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,85 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ptgoetz <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88938675
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
+ <developers>
+ <developer>
+ <id>ptgoetz</id>
+ <name>P. Taylor Goetz</name>
+ <email>ptgoetz@gmail.com</email>
+ </developer>
+ </developers>
- <artifactId>storm-hbase</artifactId>
+ <properties>
+ <hbase.version>1.1.0</hbase.version>
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88945680
--- Diff: external/storm-hbase/pom.xml ---
@@ -1,92 +1,92 @@
<?xml version="1.0" encoding="UTF-8"?>
-<!--
- Licensed to the Apache Software Foundation (ASF) under one or more
- contributor license agreements. See the NOTICE file distributed with
- this work for additional information regarding copyright ownership.
- The ASF licenses this file to You under the Apache License, Version 2.0
- (the "License"); you may not use this file except in compliance with
- the License. You may obtain a copy of the License at
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
- http://www.apache.org/licenses/LICENSE-2.0
+ <parent>
+ <artifactId>storm</artifactId>
+ <groupId>org.apache.storm</groupId>
+ <version>2.0.0-SNAPSHOT</version>
+ <relativePath>../../pom.xml</relativePath>
+ </parent>
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
- <modelVersion>4.0.0</modelVersion>
+ <artifactId>storm-hbase</artifactId>
- <parent>
- <artifactId>storm</artifactId>
- <groupId>org.apache.storm</groupId>
- <version>2.0.0-SNAPSHOT</version>
- <relativePath>../../pom.xml</relativePath>
- </parent>
--- End diff --
Parent is still there; I believe that is formatting from the IDE causing this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88940442
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,85 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
- private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseLookupBolt.class);
+
+ private HBaseValueMapper rowToTupleMapper;
+ private HBaseProjectionCriteria projectionCriteria;
+ private transient LoadingCache<byte[], Result> cache;
+ private transient boolean cacheEnabled;
--- End diff --
Why are these transient? The bolt is not multi-threaded.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88968371
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/HBaseLookupBolt.java ---
@@ -40,51 +48,85 @@
*
*/
public class HBaseLookupBolt extends AbstractHBaseBolt {
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88939598
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/AbstractHBaseBolt.java ---
@@ -37,8 +37,7 @@
public abstract class AbstractHBaseBolt extends BaseRichBolt {
private static final Logger LOG = LoggerFactory.getLogger(AbstractHBaseBolt.class);
- protected OutputCollector collector;
-
+ protected transient OutputCollector collector;
--- End diff --
Why is this transient? I don't see us accessing it from multiple threads, and especially not changing it on multiple threads?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1783
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1783: STORM-2204 Adding caching capabilities in HBaseLoo...
Posted by ambud <gi...@git.apache.org>.
Github user ambud commented on a diff in the pull request:
https://github.com/apache/storm/pull/1783#discussion_r88946112
--- Diff: external/storm-hbase/src/main/java/org/apache/storm/hbase/bolt/AbstractHBaseBolt.java ---
@@ -37,8 +37,7 @@
public abstract class AbstractHBaseBolt extends BaseRichBolt {
private static final Logger LOG = LoggerFactory.getLogger(AbstractHBaseBolt.class);
- protected OutputCollector collector;
-
+ protected transient OutputCollector collector;
--- End diff --
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.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by HeartSaVioR <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by vesense <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1783: STORM-2204 Adding caching capabilities in HBaseLookupBolt
Posted by ambud <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---