Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STORM-1880] Support EXISTS Command Storm-Redis #1479

Closed
wants to merge 1 commit into from

Conversation

darionyaphet
Copy link
Contributor

STORM-1880

add exists command in storm-redis LookupBolt

@@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes . Maybe we should rename RedisDataType to Command it's more suitable .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would recommend that if RedisDataType is internal to storm code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zscore is a function about Sorted Set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RedisMapper point out RedisDataType is hard to internal it into storm code ? I'm not sure ...

@HeartSaVioR
Copy link
Contributor

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.

@darionyaphet
Copy link
Contributor Author

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

@HeartSaVioR
Copy link
Contributor

@darionyaphet I'm curious that exists fits for use cases of Storm. Could you share your use cases?

@darionyaphet
Copy link
Contributor Author

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

@HeartSaVioR
Copy link
Contributor

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

@abhishekagarwal87
Copy link
Contributor

+1 for a filter bolt.

@darionyaphet
Copy link
Contributor Author

filter bolt seems better

@HeartSaVioR
Copy link
Contributor

OK. Filed 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?

@darionyaphet
Copy link
Contributor Author

Sure . I will close it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants