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 i
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 failu
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
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 proje
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 pr
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
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 pro
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
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
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 i
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 th
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
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 y
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
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/53630da01bcbfe05eda90869b1198b4e1c554a8
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
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 is
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 you
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
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
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 na
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
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
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 h
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 d
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
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 fe
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
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,
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 w
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 n
31 matches
Mail list logo
