-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-2861] Fields grouping on split streams fails #1387
Conversation
In |
I ran into that issue a while back. The problem was that you emit elements that have the name/tag of the split stream (to use the split selector) and over that, you run the regular partitioner. Splitting this into two steps should work: Splitting, having a mapper that removes the "split tag" and then doing the field partitioning. Would be nice to make this nice in Flink, but it is tricky. Not all functions go though a collector, and not all collectors can split. |
I don't understand you comment? What you describe is exactly how it works (even before this PR). The problem was actually just a missing |
Btw: Travis fails due to a instable test -- I already create a JIRA for it. |
this.numberOfAttributes, flinkCollector)); | ||
} | ||
final OutputCollector stormCollector = new OutputCollector(new BoltCollector<OUT>( | ||
this.numberOfAttributes, flinkCollector)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I fixed that in my code base as well...
Maybe a stupid question but why don't you send the operator name along with the BoltWrapper? Then there is no need to extract it from the Flink task name. |
f31c895
to
7d6b4c7
Compare
That's actually not a stupid question. I did not do it in the first place to avoid "redundant" code (I was not aware that Flink changes names). I just changed it to your suggestion. If Travis passed, I would merge this. Or are there any objections? |
@mjsax Okay, I probably misread the original code. I thought the |
@StephanEwen In the original code that was actually the case. The Merging this now... |
Ah, okay, so I skipped a version by accident ;-) I think that original change could not quite have worked, as it would have propagated the wrong TypeInfos to the ArrayKeySelector and OutputSerializer. The one with |
No description provided.