Github user InfinitiesLoop commented on the issue:
https://github.com/apache/flink/pull/3314
Sorry for necro'ing this thread, but where does the community land on the
multiple record per kafka payload idea this PR originally intended to solve?
I have this scenario, where a
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3314
Yes totally agree. Thanks very much for taking the time to review the PRs.
Will do it next time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3314
@haohui - one suggestion for future contributions for easier reviews:
We usually use follow-up commits that addresses review comments, instead of
force pushing the whole branch. For reviewers,
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3314
I've made some final general improvements in
https://github.com/tzulitai/flink/tree/PR-FLINK-3679.
Doing a Travis run before merging:
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/3314
LGTM, I'll proceed to merge this later today.
One minor problem: the offset state still isn't updated if `record ==
null`. We need to do the checking in the synchronize block in the
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3314
CI failed in one of the group as the group was timed out. The specific
group was not timed out in the last run.
@tzulitai can you please take another look? Thanks
---
If your project is set
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/3314
The change looks good to merge in my opinion.
@tzulitai can you also have a quick look?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/3314
Thanks a lot for your understanding @haohui.
Let us know once you've updated the PR so that we can review and merge it.
---
If your project is set up for it, you can reply to this email and
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3314
Thanks for the comments. Allowing `DeserializationSchema` to return `null`
sounds good to me. I'll update the PR accordingly.
---
If your project is set up for it, you can reply to this email and
Github user jgrier commented on the issue:
https://github.com/apache/flink/pull/3314
I think it would be just fine if we allowed a null return given the
tradeoffs discussed here. The main thing was to allow users a way to deal with
bad data with minimal effort and without throwing
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/3314
@StephanEwen and I just had an offline discussion about the change, and we
came up with the following thoughts:
Using an `ArrayList` for buffering elements is an "anti-pattern" in Flink,
Github user haohui commented on the issue:
https://github.com/apache/flink/pull/3314
@rmetzger ping...
just wondering what do you think about all the approaches we have discussed
here? Your comments are appreciated.
---
If your project is set up for it, you can reply to this
Github user rmetzger commented on the issue:
https://github.com/apache/flink/pull/3314
Thank you for opening a pull request.
I think the change is missing an update to the documentation. I did a very
very superficial review of the change :) This needs a more thorough check.
---
13 matches
Mail list logo
