[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-75576248 +1, sorry it took so long to get in. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/365 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-73170761 @HeartSaVioR , @ptgoetz I agree with committer sponsors only committers can join external's committer sponsors. I have changed the committer sponsors members in Readme, @ptgoetz , please help to review. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-72779122 @dashengju According to #416, we should remove ourselves from committer sponsors to let storm-redis module can be merged into master. Thanks for clarification, @ptgoetz. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-72309792 I wish to hear additional opinions, and finally see it pulled in master. :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-71313044 I'll address follow-up issue, providing some persist / lookup bolts after merged to master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-71273044 +1 the changes still look good Are there any other committers that want to take a look at the code? I would feel more comfortable with more people looking at the code, just because it is so large. If no one wants to take a look I guess I'll just pull it in sometime next week. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70936224 @revans2 @dashengju Just I'm confused about 'Committer Sponsor'. Is it explained from docs? I can't find it. If it doesn't exist, could you explain it? Maybe it's better to have description about 'Committer Sponsor' to BYLAWS.md. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70978681 @dashengju No, I mean that What is Committer Sponsor and where can I find description?. I think if it is Committer Sponsor then we cannot be included, but if it is Committer Sponsor then we can be included. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23232733 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/trident/state/RedisClusterMapState.java --- @@ -0,0 +1,295 @@ +/** + * 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. + */ +package org.apache.storm.redis.trident.state; + +import backtype.storm.task.IMetricsContext; +import backtype.storm.tuple.Values; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import org.apache.storm.redis.util.config.JedisClusterConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import redis.clients.jedis.JedisCluster; +import storm.trident.state.JSONNonTransactionalSerializer; +import storm.trident.state.JSONOpaqueSerializer; +import storm.trident.state.JSONTransactionalSerializer; +import storm.trident.state.OpaqueValue; +import storm.trident.state.Serializer; +import storm.trident.state.State; +import storm.trident.state.StateFactory; +import storm.trident.state.StateType; +import storm.trident.state.TransactionalValue; +import storm.trident.state.map.CachedMap; +import storm.trident.state.map.IBackingMap; +import storm.trident.state.map.MapState; +import storm.trident.state.map.NonTransactionalMap; +import storm.trident.state.map.OpaqueMap; +import storm.trident.state.map.SnapshottableMap; +import storm.trident.state.map.TransactionalMap; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class RedisClusterMapStateT implements IBackingMapT { +private static final Logger logger = LoggerFactory.getLogger(RedisClusterMapState.class); + +private static final EnumMapStateType, Serializer DEFAULT_SERIALIZERS = Maps.newEnumMap(ImmutableMap.of( +StateType.NON_TRANSACTIONAL, new JSONNonTransactionalSerializer(), +StateType.TRANSACTIONAL, new JSONTransactionalSerializer(), +StateType.OPAQUE, new JSONOpaqueSerializer() +)); + +public static class DefaultKeyFactory implements KeyFactory { +public String build(ListObject key) { +if (key.size() != 1) +throw new RuntimeException(Default KeyFactory does not support compound keys); +return (String) key.get(0); +} +}; + +public static class OptionsT implements Serializable { +public int localCacheSize = 1000; +public String globalKey = $REDIS-MAP-STATE-GLOBAL; +KeyFactory keyFactory = null; +public SerializerT serializer = null; +public String hkey = null; +} + +public static interface KeyFactory extends Serializable { +String build(ListObject key); +} + +/** + * OpaqueTransactional for redis-cluster. + * */ +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig) { +return opaque(jedisClusterConfig, new Options()); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, String hkey) { +Options opts = new Options(); +opts.hkey = hkey; +return opaque(jedisClusterConfig, opts); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, KeyFactory factory) { +Options opts = new Options(); +opts.keyFactory = factory; +return opaque(jedisClusterConfig, opts); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, OptionsOpaqueValue opts) { +return new Factory(jedisClusterConfig, StateType.OPAQUE, opts); +} + +/** + *
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user revans2 commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23232475 --- Diff: external/storm-redis/pom.xml --- @@ -0,0 +1,108 @@ +?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 + + 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; +modelVersion4.0.0/modelVersion + +parent +artifactIdstorm/artifactId +groupIdorg.apache.storm/groupId +version0.10.0-SNAPSHOT/version +relativePath../../pom.xml/relativePath +/parent + +artifactIdstorm-redis/artifactId + +developers +developer +iddashengju/id +nameDasheng Ju/name +emaildashen...@gmail.com/email +/developer +developer +idHeartSaVioR/id +nameJungtaek Lim/name +emailkabh...@gmail.com/email +/developer +/developers + +properties +jedis.version2.6.2/jedis.version +/properties + +dependencies +dependency +groupIdorg.apache.storm/groupId +artifactIdstorm-core/artifactId +version${project.version}/version +scopeprovided/scope +/dependency +dependency +groupIdredis.clients/groupId +artifactIdjedis/artifactId +version${jedis.version}/version +/dependency +dependency +groupIdcom.google.guava/groupId +artifactIdguava/artifactId +version18.0/version +/dependency +/dependencies + +!-- +build +sourceDirectorysrc/sourceDirectory +resources +resource +directory${basedir}/multilang/directory +/resource +resource +directory${basedir}/resources/directory +/resource +/resources + +plugins +plugin +groupIdorg.apache.maven.plugins/groupId +artifactIdmaven-shade-plugin/artifactId +version2.2/version +configuration +artifactSet +excludes +excludejunit:junit/exclude +excludeorg.slf4j:slf4j-simple/exclude +excludeorg.slf4j:slf4j-log4j12/exclude +excludeorg.apache.zookeeper/exclude +/excludes +/artifactSet +/configuration +executions +execution +phasepackage/phase +goals +goalshade/goal +/goals +configuration + finalName${project.artifactId}-${project.version}-with-dependencies/finalName +/configuration +/execution +/executions +/plugin +/plugins +/build +-- --- End diff -- I assume we can delete these commented parts of the pom? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23264315 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/trident/state/RedisClusterMapState.java --- @@ -0,0 +1,295 @@ +/** + * 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. + */ +package org.apache.storm.redis.trident.state; + +import backtype.storm.task.IMetricsContext; +import backtype.storm.tuple.Values; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import org.apache.storm.redis.util.config.JedisClusterConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import redis.clients.jedis.JedisCluster; +import storm.trident.state.JSONNonTransactionalSerializer; +import storm.trident.state.JSONOpaqueSerializer; +import storm.trident.state.JSONTransactionalSerializer; +import storm.trident.state.OpaqueValue; +import storm.trident.state.Serializer; +import storm.trident.state.State; +import storm.trident.state.StateFactory; +import storm.trident.state.StateType; +import storm.trident.state.TransactionalValue; +import storm.trident.state.map.CachedMap; +import storm.trident.state.map.IBackingMap; +import storm.trident.state.map.MapState; +import storm.trident.state.map.NonTransactionalMap; +import storm.trident.state.map.OpaqueMap; +import storm.trident.state.map.SnapshottableMap; +import storm.trident.state.map.TransactionalMap; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class RedisClusterMapStateT implements IBackingMapT { +private static final Logger logger = LoggerFactory.getLogger(RedisClusterMapState.class); + +private static final EnumMapStateType, Serializer DEFAULT_SERIALIZERS = Maps.newEnumMap(ImmutableMap.of( +StateType.NON_TRANSACTIONAL, new JSONNonTransactionalSerializer(), +StateType.TRANSACTIONAL, new JSONTransactionalSerializer(), +StateType.OPAQUE, new JSONOpaqueSerializer() +)); + +public static class DefaultKeyFactory implements KeyFactory { +public String build(ListObject key) { +if (key.size() != 1) +throw new RuntimeException(Default KeyFactory does not support compound keys); +return (String) key.get(0); +} +}; + +public static class OptionsT implements Serializable { +public int localCacheSize = 1000; +public String globalKey = $REDIS-MAP-STATE-GLOBAL; +KeyFactory keyFactory = null; +public SerializerT serializer = null; +public String hkey = null; +} + +public static interface KeyFactory extends Serializable { +String build(ListObject key); +} + +/** + * OpaqueTransactional for redis-cluster. + * */ +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig) { +return opaque(jedisClusterConfig, new Options()); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, String hkey) { +Options opts = new Options(); +opts.hkey = hkey; +return opaque(jedisClusterConfig, opts); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, KeyFactory factory) { +Options opts = new Options(); +opts.keyFactory = factory; +return opaque(jedisClusterConfig, opts); +} + +public static StateFactory opaque(JedisClusterConfig jedisClusterConfig, OptionsOpaqueValue opts) { +return new Factory(jedisClusterConfig, StateType.OPAQUE, opts); +} + +/** +
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user Parth-Brahmbhatt commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70752983 @HeartSaVioR and @dashengju Thanks for the patch and I think overall the code looks good. I am little confused about the core bolt. You have a trident state and query function but in the core implementation you just provide an abstract bolt. Any reason why the core functionality can not be extended to support the basic persist and lookup features out of the box? You already have a mapper interface for trident and instead of taking the tridentuple as input you could change the type to ITuple so it will be able to support both trident states and core bolt. It is ok to leave this implementation as is and file a jira to add the core persist and lookup bolts later or document why it is not a good idea to do so. Also I think the README should be more detailed. you probably want to document that users trying to use core features needs to extend AbstractRedisBolt and how they should be using methods from the parent class. The trident section should cover the query functions and your probably want to remove ShardedRedis related classes from README as they don't exist. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70755347 @Parth-Brahmbhatt Thanks for review! First of all, let's start about abstract bolt. https://github.com/apache/storm/pull/365#issuecomment-68332013 You can see the matrix I've commented. Redis provides so many data types so we need to decide what data types we support or not. I thought JedisCommands is easy and convenient enough to use so users would no problem to extend abstract bolt and make his/her own bolt. But I'm also thinking it is really better for users to force making bolt, instead of providing some simple bolts. It could be already inconvenient to use in point of users' view. @revans2 @dashengju How do you think about? Btw, persist/lookup bolts for some datatypes can be added at any time. Also, changing mapper to support normal bolt and trident seems great. We can check it now, and provide some bolts based on it if it works smoothly. Thanks for your help! Then let's talk about README.md. Describing usage of AbstractBolt to README.md is already done, please see AbstractRedisBolt usage. I agree we should remove description related to SharededJedis. I'll take care of it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70752747 @dashengju For your information, Jedis team is discussing about Jedis 3.0, and ShardedJedis could be replaced to JedisCluster for reducing code complexity. https://groups.google.com/d/msg/jedis_redis/7ZJPFp_qTY0/ISHqEPZlAnwJ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r23262904 --- Diff: external/storm-redis/pom.xml --- @@ -0,0 +1,108 @@ +?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 + + 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; +modelVersion4.0.0/modelVersion + +parent +artifactIdstorm/artifactId +groupIdorg.apache.storm/groupId +version0.10.0-SNAPSHOT/version +relativePath../../pom.xml/relativePath +/parent + +artifactIdstorm-redis/artifactId + +developers +developer +iddashengju/id +nameDasheng Ju/name +emaildashen...@gmail.com/email +/developer +developer +idHeartSaVioR/id +nameJungtaek Lim/name +emailkabh...@gmail.com/email +/developer +/developers + +properties +jedis.version2.6.2/jedis.version +/properties + +dependencies +dependency +groupIdorg.apache.storm/groupId +artifactIdstorm-core/artifactId +version${project.version}/version +scopeprovided/scope +/dependency +dependency +groupIdredis.clients/groupId +artifactIdjedis/artifactId +version${jedis.version}/version +/dependency +dependency +groupIdcom.google.guava/groupId +artifactIdguava/artifactId +version18.0/version +/dependency +/dependencies + +!-- +build +sourceDirectorysrc/sourceDirectory +resources +resource +directory${basedir}/multilang/directory +/resource +resource +directory${basedir}/resources/directory +/resource +/resources + +plugins +plugin +groupIdorg.apache.maven.plugins/groupId +artifactIdmaven-shade-plugin/artifactId +version2.2/version +configuration +artifactSet +excludes +excludejunit:junit/exclude +excludeorg.slf4j:slf4j-simple/exclude +excludeorg.slf4j:slf4j-log4j12/exclude +excludeorg.apache.zookeeper/exclude +/excludes +/artifactSet +/configuration +executions +execution +phasepackage/phase +goals +goalshade/goal +/goals +configuration + finalName${project.artifactId}-${project.version}-with-dependencies/finalName +/configuration +/execution +/executions +/plugin +/plugins +/build +-- --- End diff -- @revans2 So do I. I can take care of it and send PR to @dashengju 's repo. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70777681 @revans2 , I have added you to the committer. thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-70575248 @harshach @revans2 @ptgoetz I'd like to hear your thoughts about this. Anything including code reviews, thought about Storm + Redis, and so on is very appreciated. Thanks in advance! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68803852 @dashengju Great! Thanks for applying. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68626207 @HeartSaVioR and I have added abstract bolt and trident state implementations, we also add some test cases, and modify the readme usage. Anyone can help review? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r22443235 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/trident/state/RedisClusterStateUpdater.java --- @@ -0,0 +1,76 @@ +/** + * 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. + */ +package org.apache.storm.redis.trident.state; + +import org.apache.storm.redis.trident.mapper.TridentTupleMapper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import redis.clients.jedis.JedisCluster; +import storm.trident.operation.TridentCollector; +import storm.trident.state.BaseStateUpdater; +import storm.trident.tuple.TridentTuple; + +import java.util.List; + +public class RedisClusterStateUpdater extends BaseStateUpdaterRedisClusterState { +private static final Logger logger = LoggerFactory.getLogger(RedisClusterState.class); + +private static final long DEFAULT_EXPIRE_INTERVAL_MS = 8640; + +private final String redisKeyPrefix; +private final TridentTupleMapper tupleMapper; +private final long expireIntervalMs; + +public RedisClusterStateUpdater(String redisKeyPrefix, TridentTupleMapper tupleMapper, long expireIntervalMs) { +this.redisKeyPrefix = redisKeyPrefix; +this.tupleMapper = tupleMapper; +if (expireIntervalMs 0) { +this.expireIntervalMs = expireIntervalMs; +} else { +this.expireIntervalMs = DEFAULT_EXPIRE_INTERVAL_MS; +} +} + +@Override +public void updateState(RedisClusterState redisClusterState, ListTridentTuple inputs, +TridentCollector collector) { +long expireAt = System.currentTimeMillis() + expireIntervalMs; + +JedisCluster jedisCluster = null; +try { +jedisCluster = redisClusterState.getJedisCluster(); +for (TridentTuple input : inputs) { +String key = this.tupleMapper.getKeyFromTridentTuple(input); +String redisKey = key; +if (redisKeyPrefix != null redisKeyPrefix.length() 0) { +redisKey = redisKeyPrefix + redisKey; +} +String value = this.tupleMapper.getValueFromTridentTuple(input); + +logger.debug(update key[ + key + ] redisKey[ + redisKey + ] value[ + value + ]); + +jedisCluster.set(redisKey, value); --- End diff -- Just a note, we can use set command with ex parameter or setex command to reduce round-trip. http://redis.io/commands/set --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on a diff in the pull request: https://github.com/apache/storm/pull/365#discussion_r22443267 --- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/trident/state/RedisMapState.java --- @@ -0,0 +1,327 @@ +/** + * 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. + */ +package org.apache.storm.redis.trident.state; + +import backtype.storm.task.IMetricsContext; +import backtype.storm.tuple.Values; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; +import org.apache.storm.redis.util.config.JedisPoolConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import redis.clients.jedis.Jedis; +import redis.clients.jedis.JedisPool; +import redis.clients.jedis.Pipeline; +import storm.trident.state.JSONNonTransactionalSerializer; +import storm.trident.state.JSONOpaqueSerializer; +import storm.trident.state.JSONTransactionalSerializer; +import storm.trident.state.OpaqueValue; +import storm.trident.state.Serializer; +import storm.trident.state.State; +import storm.trident.state.StateFactory; +import storm.trident.state.StateType; +import storm.trident.state.TransactionalValue; +import storm.trident.state.map.CachedMap; +import storm.trident.state.map.IBackingMap; +import storm.trident.state.map.MapState; +import storm.trident.state.map.NonTransactionalMap; +import storm.trident.state.map.OpaqueMap; +import storm.trident.state.map.SnapshottableMap; +import storm.trident.state.map.TransactionalMap; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; +import java.util.List; +import java.util.Map; + +public class RedisMapStateT implements IBackingMapT { +private static final Logger logger = LoggerFactory.getLogger(RedisMapState.class); + +private static final EnumMapStateType, Serializer DEFAULT_SERIALIZERS = Maps.newEnumMap(ImmutableMap.of( +StateType.NON_TRANSACTIONAL, new JSONNonTransactionalSerializer(), +StateType.TRANSACTIONAL, new JSONTransactionalSerializer(), +StateType.OPAQUE, new JSONOpaqueSerializer() +)); + +public static class DefaultKeyFactory implements KeyFactory { +public String build(ListObject key) { +if (key.size() != 1) +throw new RuntimeException(Default KeyFactory does not support compound keys); +return (String) key.get(0); +} +}; + +public static class OptionsT implements Serializable { +public int localCacheSize = 1000; +public String globalKey = $REDIS-MAP-STATE-GLOBAL; +KeyFactory keyFactory = null; +public SerializerT serializer = null; +public String hkey = null; +} + +public static interface KeyFactory extends Serializable { +String build(ListObject key); +} + +/** + * OpaqueTransactional for redis. + * */ +public static StateFactory opaque(JedisPoolConfig jedisPoolConfig) { +return opaque(jedisPoolConfig, new Options()); +} + +public static StateFactory opaque(JedisPoolConfig jedisPoolConfig, String hkey) { +Options opts = new Options(); +opts.hkey = hkey; +return opaque(jedisPoolConfig, opts); +} + +public static StateFactory opaque(JedisPoolConfig jedisPoolConfig, KeyFactory factory) { +Options opts = new Options(); +opts.keyFactory = factory; +return opaque(jedisPoolConfig, opts); +} + +public static StateFactory opaque(JedisPoolConfig jedisPoolConfig, OptionsOpaqueValue opts) { +return new Factory(jedisPoolConfig, StateType.OPAQUE, opts); +} + +/** + * Transactional for redis. + * */ +public static
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68652310 @dashengju Though there're some points to reduce round-trip to Redis, looks good to me. Great job! Seems like we're ready to review our effort to Storm community and finally merge into Storm Project. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68431816 @harshach , we have added tests for both topology and trident. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68431920 @dashengju JedisCommands is for single key. JedisCluster supports multi keys operation but all keys should be in one slot (so it should be handled differently from Jedis), and ShardedJedis doesn't support multi keys operation. That's what I suggest we can use JedisCommands and unify all things 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68434697 @HeartSaVioR If JedisCommands is designed for single key operation, It is useful for RedisState, because we provide the JedisCommands interface to user and they can unify all things into one. It is not suitable for RedisMapState, because MapState do not expose JedisCommands interface to user. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68437760 @dashengju If we cannot tolerate single key operations restriction, there're some ways to settle. 1. check type and convert, from usage of Jedis We can check type (as you did) and convert JedisCommands to Jedis if available. pros. It's simplest way cons. Both handling Jedis and original (ex. Trident State) logic got mixed. 2. introduce CommandExecutor We can introduce CommandExecutor, and passes Jedis/JedisCluster into it. CommandExecutor implements and exposes available operations, such as get / set / mget / mset / hset / hget / etc. (ex. If Jedis is passed, CommandExecutor.mget() calls actual mget(), but if JedisCluster is passed, CommandExecutor.mget() runs loop and calls get() for each key. ) pros. We can separate handling Jedis and original logic. cons. We have to implement additional component, which adds complexity. 3. Drop support for ShardedJedis, JedisCluster We can drop support for ShardedJedis and JedisCluster, and postpone this issue. pros. There're no limit to use Jedis directly. We can use Pipeline if we need higher speed and requests could be batched. cons. State repository is restricted to one instance of Redis. Please add any alternative approaches or let me hear your opinions. :D Surely anyone can participate this discussion. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68447314 The users of our trident state, they may have a redis or have a redis cluster. normally, they will not have both. When they have a redis, they want to use the Jedis client for more function, such as mget/mset. they do not care about JedisCluster. When they have a redis cluster, they can just use the JedisCluster client for single operation. As redis bolt, because it handle one tuple one time, we provide JedisCommands is Ok. As trident state, how about we support more implementations? 1) TridentState/TridentMapState, which provide JedisCommands interface both for single redis or redis cluster. 2) JedisTridentState/JedisTridentMapState, which provide Jedis interface just for single redis. 3) JedisClusterTridentState/JedisClusterTridentMapState, which provide JedisCluster interface, just for redis cluster. 4) ShardedJedisTridentState/ShardedJedisTridentMapState, which provide ShardedJedis interface. Users can select what they need. And storm-redis can go along with jedis. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68447798 may be support 1) 2) 4) is ok --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68448629 @dashengju Supporting 2, 3, 4 seems OK. When we need 2 and/or 3, and/or 4 separately, I don't think we need to maintain 1. 1 is just for unifying. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68449366 okï¼I will add 2) 3) 4) those days. @HeartSaVioR Happy New Year :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68450496 @dashengju Thanks! Happy new year! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68341375 @dashengju It would be great to move test codes to src/test/java. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68346400 @HeartSaVioR I have merged your PR, thanks. And I have added Apache licenses to all files. I have found JedisCommands interface suitable for trident state, and I will change my code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68426972 @dashengju Thanks for applying unification on Jedis / JedisCluster into Trident implementation! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68244243 @darionyaphet , I think storm-redis can support redis and redis-cluster both. we can also add redis-sharding solution to trident state. @HeartSaVioR , yes, I think storm-redis is first accepted, then we can work on the same place to add more modules to storm-redis. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user harshach commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68267817 @dashengju thanks for sharing this. I would like to see unit tests on the code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68322020 @dashengju Hey, it's better to collaborate with you by requesting PR to merge. I posted small PR to your fork just now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68324052 @harshach , @Parth-Brahmbhatt , @HeartSaVioR I have implemented a few functions in current PR. As @HeartSaVioR says, it's better to collaborate with each other to make this PR to merge. You are welcome to make PR to my branch. I will try to add support for core storm bolt, and add unit test for those code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68324419 @dashengju Oh, actually I'm sketching features for supporting core storm bolt. Could you wait for me to share sketches and let me implement it, or do you want your own implementation? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68324513 My sketch would be similar to storm-hbase, but because of various Redis data types, supporting types needs design before implementing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68325551 @HeartSaVioR , ok , you can responsible for core storm bolt. And I will add more data types for trident state and unit tests. next time commit, I will add you to the Committer Sponsors for storm-redis component. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68325672 @dashengju Thanks for concerning! I'll share a sketch about what it looks like (maybe similar to storm-hbase) and what types it can support. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user darionyaphet commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68327749 redis publish/subscribe is supported ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68330414 @darionyaphet Hello. For spout, it isn't acceptable for reliable data source because during failure it'll lose some tuples. (Actually Redis cannot act as reliable message queue, so I think it isn't good to implement spout for Redis.) For bolt, every bolt should react based on tuple from spout or other bolt, so pub/sub seems not fit this case. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68332013 Here're data types and commands I'm trying to support at first time. Type | Read | Write --- | --- | --- STRING | GET (key) | SET (key, value), INCRBY (key, incr) HASH | HGET (key, field) | HSET (key, field, value) LIST | LPOP (key) | RPUSH (key, value) SET | | SADD (key, member) SORTED SET | ZSCORE (key, member) | ZADD (key, score, member), ZINCRBY (key, incr, member) HLL (HyperLogLog) | PFCOUNT (key) | PFADD (key, element) There're so many candidate list, but we don't need to care all things because users may use Jedis easily. There're also many commands which retrieves subset of values (by range, all), but it's good to start with essential and expand if we think it would be great to have. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-609] Add storm-redis to storm external
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/365#issuecomment-68235170 Since Redis Cluster has some limitations, it seems better to implement it with Redis itself (and Jedis) unless state uses a big memory. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---