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
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
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
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
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
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
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
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?
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
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
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
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 @@
+/**
+ *
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
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
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.
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
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.
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
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
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?
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 @@
+/**
+ *
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
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
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
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
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
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
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
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
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
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 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
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 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
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
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
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
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
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
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
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
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
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
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
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
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,
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
47 matches
Mail list logo
