You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by darionyaphet <gi...@git.apache.org> on 2016/06/10 08:20:58 UTC
[GitHub] storm pull request #1479: [STORM-1880] Support EXISTS Command Storm-Redis
GitHub user darionyaphet opened a pull request:
https://github.com/apache/storm/pull/1479
[STORM-1880] Support EXISTS Command Storm-Redis
[STORM-1880](https://issues.apache.org/jira/browse/STORM-1880)
add exists command in storm-redis LookupBolt
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/darionyaphet/storm STORM-1880
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1479.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 #1479
----
commit 1634930e87ea488cd464248a01d89b80fe4217f5
Author: darionyaphet <da...@gmail.com>
Date: 2016-06-10T08:18:25Z
STORM-1880 : Support EXISTS Command 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on the issue:
https://github.com/apache/storm/pull/1479
@HeartSaVioR
Yes I have realized that `RedisDataType` is a series of data format . But `EXISTS` is really heavily used in Redis usage .
Should we split data and operation into difference 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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on the issue:
https://github.com/apache/storm/pull/1479
filter bolt seems better
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1479#discussion_r66595996
--- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/common/mapper/RedisDataTypeDescription.java ---
@@ -23,7 +23,7 @@
* RedisDataTypeDescription defines data type and additional key if needed for lookup / store tuples.
*/
public class RedisDataTypeDescription implements Serializable {
- public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO }
+ public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO, EXISTS }
--- End diff --
I am not fully aware of Redis terminology but EXISTS doesn't look like a data type. How do these data types relate to the redis operation. e.g. SORTED_SET and zscore?
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on the issue:
https://github.com/apache/storm/pull/1479
@HeartSaVioR check `exists` is very useful . Sometimes we will check if a key is in a key set to decide if the value should be process .
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on the issue:
https://github.com/apache/storm/pull/1479
Sure . I will close 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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1479
@darionyaphet
Instead of having `exists` as datatype, we can have a flag that indicates checking only availability. It can be optimized for STRING and HASH type with exists and hexists, and also available anyway for other types (lookup and simply check that whether it's not null or null).
Or we even have new filter Bolt that filters tuples by availability. Might be more clearer.
@darionyaphet @abhishekagarwal87 What do you think?
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1479
@darionyaphet I'm curious that `exists` fits for use cases of Storm. Could you share your use cases?
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the issue:
https://github.com/apache/storm/pull/1479
+1 for a filter 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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1479
OK. Filed [STORM-1919](https://issues.apache.org/jira/browse/STORM-1919) for FilterBolt.
We can close this and mark STORM-1880 as "Won't fix".
@darionyaphet Since I have an idea in mind how to implement this, so if you don't mind I'll work on STORM-1919. Are you OK with 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 pull request #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on a diff in the pull request:
https://github.com/apache/storm/pull/1479#discussion_r66597076
--- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/common/mapper/RedisDataTypeDescription.java ---
@@ -23,7 +23,7 @@
* RedisDataTypeDescription defines data type and additional key if needed for lookup / store tuples.
*/
public class RedisDataTypeDescription implements Serializable {
- public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO }
+ public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO, EXISTS }
--- End diff --
`zscore` is a function about `Sorted 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on a diff in the pull request:
https://github.com/apache/storm/pull/1479#discussion_r66596642
--- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/common/mapper/RedisDataTypeDescription.java ---
@@ -23,7 +23,7 @@
* RedisDataTypeDescription defines data type and additional key if needed for lookup / store tuples.
*/
public class RedisDataTypeDescription implements Serializable {
- public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO }
+ public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO, EXISTS }
--- End diff --
Yes . Maybe we should rename `RedisDataType` to `Command` it's more suitable .
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on a diff in the pull request:
https://github.com/apache/storm/pull/1479#discussion_r66598237
--- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/common/mapper/RedisDataTypeDescription.java ---
@@ -23,7 +23,7 @@
* RedisDataTypeDescription defines data type and additional key if needed for lookup / store tuples.
*/
public class RedisDataTypeDescription implements Serializable {
- public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO }
+ public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO, EXISTS }
--- End diff --
`RedisMapper` point out `RedisDataType` is hard to internal it into storm code ? I'm not sure ...
---
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 #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1479#discussion_r66596983
--- Diff: external/storm-redis/src/main/java/org/apache/storm/redis/common/mapper/RedisDataTypeDescription.java ---
@@ -23,7 +23,7 @@
* RedisDataTypeDescription defines data type and additional key if needed for lookup / store tuples.
*/
public class RedisDataTypeDescription implements Serializable {
- public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO }
+ public enum RedisDataType { STRING, HASH, LIST, SET, SORTED_SET, HYPER_LOG_LOG, GEO, EXISTS }
--- End diff --
would recommend that if RedisDataType is internal to storm 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1479
Hi @darionyaphet.
Thanks for the contribution.
Btw, RedisLookupBolt and RedisStoreBolt are intended to map tuple to Redis data or opposite (You could imagine object mapper or ORM), not intended to support operations. `EXISTS` seems not fit to intention 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 pull request #1479: [STORM-1880] Support EXISTS Command Storm-Redis
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet closed the pull request at:
https://github.com/apache/storm/pull/1479
---
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.
---