[GitHub] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 Hi @kl0u, IMO that is the expected behavior. The sink would not know that if the Redis is down or not unless it tries to send the next data to the Redis. When ever a new message reaches the sink it tries to use the connection pool, then an then only it can throw exception that it can not send the data to Redis. You can build a heartbeat mechanism to check periodically that Redis serve is up or down, and can cancel the job if Redis is down. @mjsax please correct me if my understanding is wrong. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user kl0u commented on the issue: https://github.com/apache/flink/pull/1813 Hi @subhankarb ! By playing around with the sink during testing, I saw that if Redis goes down in the middle of the execution of a job, the job has to wait until the next element (after the Redis failure) arrives in order to detect that it is down. This is not the expected behavior, as we want the job to fail as soon as Redis goes down. Do you have an idea of why this is happening or how to fix 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 Thanks @subhankarb ! Great work. Thanks @tzulitai for helping with reviewing! --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/1813 This is the build: https://travis-ci.org/rmetzger/flink/builds/142738735 --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/1813 I'm addressing my last comments myself and merge the change once its green. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @rmetzger LGTM. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax , @rmetzger plz review. IMO it is ready to get merged at last :) --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax @subhankarb: the changes look good 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @subhankarb two tiny comments @tzulitai @rmetzger Any more comments? --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @subhankarb I think one more pass and we are good to go! --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Hi @subhankarb, Thank you for addressing our comments. I have some last nit-pick comments to make the code just a little better, otherwise the changes LGTM :) Perhaps we should also wait for @mjsax to give another review on the last changes too, before you get on to addressing the latest comments. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 @subhankarb We should also add Redis Sink to the fault tolerance guarantee table for the connectors in the documentation. It can be found at `flink/docs/api/streaming/fault_tolerance.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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Hi @mjsax @subhankarb, Gave a first run-through on the code, please let me know if you have any questions on the comments. I've also tested the connector again, on a local single-node & cluster installation, seems to work fine. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax I think the failing `JMXReporterTest.testJMXAvailability` was just hotfixed with this commit yesterday: https://github.com/apache/flink/commit/53630da01bcbfe05eda90869b1198b4e1c554a86 --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Sure, I'll give a full review 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @rmetzger What about this failing tests... ``` JMXReporterTest.testJMXAvailability:148 » Runtime Could not start JMX server o... ``` Seems, there is no JIRA -- known issue? -- or no issue? --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax , @rmetzger plz review. The changed model is described in the PR description. thanks, subhankar --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 My two cents: 1) seems to got sorted out (thx @tzulitai for the input!) 2) I personally do not care too much about the name conflict. Reusing the same class for sink and source sounds reasonable. Maybe `FlinkRedisConfig` as name? 3) Agreed. `PUBSUB` as datatpye is fine IMHO (so we can use same type for source and sink, which makes it clear if PUB or SUB is used) 4 + 5 ) Your suggestions sound good to me. Please update PR so we can get better lock and feel for it :) @tzulitai @subhankarb Thanks a lot! --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Hi @subhankarb, Sorry, I just read the code and didn't realize you were using the `additionalKey` as the `setName` for `SORTED_SET`. I think this might be quite confusing for Redis users. For Redis users, for sorted sets the `setName` is considered as the primary key, and the per-value score in the set is considered the additional / secondary key. It works the same for the hash data type: the hash fields are considered additional / secondary keys. So right now, the usage principal for `SORTED_SET` and `HASH` seems to be opposite to each other? --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue:
https://github.com/apache/flink/pull/1813
@tzulitai, @mjsax thank you very much for your valuable feedback.
1) `additionalKey` was supposed to one time declaration for `SORTED_SET`
and `HASH`. For `HASH`
it is the value of hash name and `SORTED_SET` it is the name of the set. I
assume it would not change once it declares.
2) `JedisPoolConfig`/ `JedisClusterConfig` were not `Serializable` so i
needed a wrapper class for that. @mjsax there is a story for a source for
Redis, so `RedisSinkConfig` would not be a valid one as the config would be
same for sink and source. Thats why i make the `JedisPoolConfig`/
`JedisClusterConfig` ctor private. So that user forced to use the builder class
to avoid confusion.
3) When i started with this i thought it was obvious that if i use PUBSUB
in sink, i would always use it for publish and if i use this in source i would
always use it for subscribe :) .
4 + 5 ) We can group the command and dataType like.
`public enum RedisCommand {
LPUSH(RedisDataType.LIST),
RPUSH(RedisDataType.LIST);
private RedisDataType redisDataType;
RedisCommand(RedisDataType redisDataType) {
this.redisDataType = redisDataType;
}
public boolean isInRedisDataType(RedisDataType redisDataType) {
return this.redisDataType == redisDataType;
}
}`
And in `RedisDataTypeDescription` we can extract the command . So that in
future we can add new command.
---
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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Hi @mjsax, Regarding 1): just came to me that for SORTED_SET, a fixed secondary key doesn't make sense, because the secondary key is supposed to define the order of the inserted element in the set. For HASH, I guess it depends on the use case. For example, a stream of user events with user id can update Redis using the user id as primary key, and inner hash field (secondary key) depending on what event occurred. I agree with your suggestion to settle with a subset of commands to start with, and if we want to, make it more flexible for multiple command per data type with new JIRAs as it would probably need larger refactoring. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @tzulitai Thank a lot for testing this! Your feedback is really great! 1) I am not a Redis users either -- from my understanding, the second key determined the field that would be updated -- it makes sense to me, that this field is the same for all updates (ie, independent of the data itself). Thus, it would not make sense to extract it from the record via `RedisMapper`. 2) Might be confusing for Redis users (I did not stumble over it as a no-user ;)) -- renaming seems reasonable -- maybe `RedisSinkConfig` ? 3+4) Well, the data type is the same, no matter if you want to read or write it. But I agree, that using the date type itself in `RedisSink` might not be sufficient. I think the idea to support `LPUSH` was, that Flink as a streaming system might do best to append at the tail, not at the head... 5) Similar to my answer to (4) -- the data type is the same -- independently what operation you perform on it. And you state yourself, that `RedisMapper` maps the type to the command/action. 6) See my answer to (1) As a non-Redis users, it is hard for me to judge what flexibility is required/expected by users (ie, secondary key per record or not?). It also seems, that not all available action (LPUSH vs RPUSH) are implemented. How important is it, to support all action -- do you think, that the current once cover the most basic subset to get started with. It might be good, to just start with a subset of commands, and add new commands later on (just a suggestion). And do the think the implemented actions for each type are the most useful? If we want to support multiple different actions per data type, it would be a larger refactoring of this code I think. @subhankarb @rmetzger What is your opinion on 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/1813 Hi, I've tested this on a local single node installation & cluster installation. It works and have not come across problems. Have a few other questions on the usage of the connector (I haven't went through the code thoroughly enough and am not an expert Redis user, so I apologize if I may be missing things in asking them). Most of them are my own opinion from a user's point of view: 1) as a user, I think the current way of giving secondary key for `HASH` and `SORTED_SET` is a bit confusing. Is the current design a common usage pattern across Redis client libraries? If not, IMO, perhaps a `getSecondaryKeyFromData` in the `RedisMapper` interface will be more explicit. 2) I wonder what's the reason for having our own `JedisPoolConfig`, when Jedis has its own `JedisPoolConfig`? If there's a need for it for Flink-specific `RedisSink` settings, perhaps the name conflict is a bit misleading? 3) Would it be better to have a `RedisDataType.PUB` instead of `RedisDataType.PUBSUB` to better acknowledge the `publish` method on Jedis clients? We can have `RedisDataType.SUB` when we add a Redis source to the connector. 4) Can `RedisDataType.LIST` be changed to `RedisDataType.LPUSH` and `RedisDataType.RPUSH`? If it isn't too hard to add, the connector would be much more useful with this. 5) In general, I have the feeling that `RedisDataType` should be named as `RedisCommandType`, as the `RedisMapper` is actually mapping the incoming data to a Redis command / action. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 @rmetzger Thanks :) I did not test with a Redis cluster or similar. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax, did you test the code with a redis installation / on a cluster? (I'm not expecting those tests from a PR review, but I would not do it again if you already did ... and I'm not doing it today ;) ) --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 Please address last comment. Otherwise, LGTM. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax plz 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user subhankarb commented on the issue: https://github.com/apache/flink/pull/1813 @mjsax wow... that's a lot. thanks you very very much for your time. i am fixing ASAP. --- 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] flink issue #1813: [FLINK-3034] Redis Sink Connector
Github user mjsax commented on the issue: https://github.com/apache/flink/pull/1813 Most comments are nit or minor. Please fix. Otherwise, looks good. --- 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. ---
