[GitHub] [flink] zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] Build ResultSubpartitionInfo and InputChannelInfo in respective constructors

2020-03-15 Thread GitBox 
zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] 
Build ResultSubpartitionInfo and InputChannelInfo in respective constructors
URL: https://github.com/apache/flink/pull/11400#discussion_r39241
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/NettyShuffleEnvironment.java
 ##
 @@ -217,6 +218,7 @@ public ShuffleIOOwnerContext createShuffleIOOwnerContext(
for (InputGateDeploymentDescriptor igdd : 
inputGateDeploymentDescriptors) {
SingleInputGate inputGate = 
singleInputGateFactory.create(
ownerContext.getOwnerName(),
+   counter,
 
 Review comment:
   I thought you were referring to the unnecessary transformation in the logics 
before, sorry for misunderstanding.
   
   You are right that the current `Collection` actually does not provide any 
order guarantee which relies on the subclass interface (`List` or `Set`), and 
we indeed need the determined order after restarting.
   
   It is reasonable to adjust the `Collection` to `List` as you suggested, but 
the only trouble is that it would change the public interface of 
`ShuffleEnvironment` proposed in FLIP-31. So it might break the compatibility 
if some outside users already relied on it. I guess it needs to discussion in 
ML for finalization.  Anyway, I already addressed this issue in a separate 
commit.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] Build ResultSubpartitionInfo and InputChannelInfo in respective constructors

2020-03-13 Thread GitBox 
zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] 
Build ResultSubpartitionInfo and InputChannelInfo in respective constructors
URL: https://github.com/apache/flink/pull/11400#discussion_r392247611
 
 

 ##
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/NettyShuffleEnvironment.java
 ##
 @@ -217,6 +218,7 @@ public ShuffleIOOwnerContext createShuffleIOOwnerContext(
for (InputGateDeploymentDescriptor igdd : 
inputGateDeploymentDescriptors) {
SingleInputGate inputGate = 
singleInputGateFactory.create(
ownerContext.getOwnerName(),
+   counter,
 
 Review comment:
   I guess your suggestion is for unifying all the related data structures 
(collection, array) from `TaskDeploymentDescriptor.inputGates` to `List`?
   
   Actually I do not see any correctness or performance problems ATM. This 
unification really spans cross multiple components from scheduler to task 
stack, and I do think it is really necessary to refactor it now. Or if we want 
to do that, it should be a separate ticket out of this PR scope. We only make 
use of previous `counter` to give a correct gate index while creating in 
iteration for this PR motivation. WDYT?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] Build ResultSubpartitionInfo and InputChannelInfo in respective constructors

2020-03-13 Thread GitBox 
zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] 
Build ResultSubpartitionInfo and InputChannelInfo in respective constructors
URL: https://github.com/apache/flink/pull/11400#discussion_r392117548
 
 

 ##
 File path: 
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/consumer/SingleInputGateTest.java
 ##
 @@ -60,10 +61,7 @@
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;
 
 Review comment:
   Sure, I was not aware that my local IDE did it automatically. :(


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] Build ResultSubpartitionInfo and InputChannelInfo in respective constructors

2020-03-13 Thread GitBox 
zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] 
Build ResultSubpartitionInfo and InputChannelInfo in respective constructors
URL: https://github.com/apache/flink/pull/11400#discussion_r392122559
 
 

 ##
 File path: 
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/ResultPartitionBuilder.java
 ##
 @@ -40,6 +40,8 @@
 
private BoundedBlockingSubpartitionType blockingSubpartitionType = 
BoundedBlockingSubpartitionType.AUTO;
 
+   private int partitionIndex = 0;
 
 Review comment:
   This builder is only for testing purpose, and I do not think this property 
is more important or very necessary than other existing fields like 
`partitionId`, `numberOfSubpartitions` etc. Even I guess this new field is rare 
used for most of the testing cases, so I prefer to not making it as mandatory.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] Build ResultSubpartitionInfo and InputChannelInfo in respective constructors

2020-03-13 Thread GitBox 
zhijiangW commented on a change in pull request #11400: [FLINK-16586][network] 
Build ResultSubpartitionInfo and InputChannelInfo in respective constructors
URL: https://github.com/apache/flink/pull/11400#discussion_r392117548
 
 

 ##
 File path: 
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/partition/consumer/SingleInputGateTest.java
 ##
 @@ -60,10 +61,7 @@
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;
 
 Review comment:
   Sure, my local IDE did it automatically.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services