Github user mjsax commented on a diff in the pull request:

    https://github.com/apache/flink/pull/1398#discussion_r45758926
  
    --- Diff: 
flink-contrib/flink-storm/src/test/java/org/apache/flink/storm/wrappers/WrapperSetupHelperTest.java
 ---
    @@ -180,8 +178,6 @@ public void testCreateTopologyContext() {
                builder.setBolt("bolt2", (IRichBolt) operators.get("bolt2"), 
dops.get("bolt2")).allGrouping("spout2");
                builder.setBolt("sink", (IRichBolt) operators.get("sink"), 
dops.get("sink"))
                                .shuffleGrouping("bolt1", 
TestDummyBolt.groupingStreamId)
    -                           .shuffleGrouping("bolt1", 
TestDummyBolt.shuffleStreamId)
    -                           .shuffleGrouping("bolt2", 
TestDummyBolt.groupingStreamId)
                                .shuffleGrouping("bolt2", 
TestDummyBolt.shuffleStreamId);
    --- End diff --
    
    Don't understand me wrong. I don't want to discard your work! And I believe 
that you did not intent do get a "messy" PR. But that's the current state.
    
    I think we can refine and merge it. But it does not resolve FLINK-2837 even 
if it improves on it. I would also assume, that your union code will be 
reworked heavily later on... Not sure about your tuple meta information code. 
Need to have a look in detail. That is the reason why I had the idea to apply 
the discussed API changes only in a single PR. But if this is too complex, we 
should just carry on with this PR.
    
    Btw: even if the JIRA is quite old it is not assigned to you; thus you 
should have ask about it. You did the same with FLINK-2837 which was assigned 
to me, too -- I did not work in it yet so a assigned it to you (I thought as 
you did have the union code together with the API changes, that should be fine).
    
    Additionally, the reason I just assigned it to you was, that FLINK-2837 is 
actually a requirement for FLINK-2721. That is why I stopped working on it back 
than, but did not have time to fix FLINK-2837 either. I did not assume that you 
tackle the join-case which does require the tuple meta info... A regular union 
does not require it.
    
    Anyway. Just let us get this PR 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 [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to