[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/5583 ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user pnowojski commented on a diff in the pull request: https://github.com/apache/flink/pull/5583#discussion_r171174932 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/UnionInputGate.java --- @@ -189,11 +189,11 @@ public void requestPartitions() throws IOException, InterruptedException { bufferOrEvent.setChannelIndex(channelIndexOffset + bufferOrEvent.getChannelIndex()); - return Optional.ofNullable(bufferOrEvent); + return Optional.of(bufferOrEvent); } @Override - public Optional pollNextBufferOrEvent() throws IOException, InterruptedException { + public Optional pollNextBufferOrEvent() throws UnsupportedOperationException { --- End diff -- as you prefer ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user NicoK commented on a diff in the pull request: https://github.com/apache/flink/pull/5583#discussion_r170988543 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/UnionInputGate.java --- @@ -61,7 +61,7 @@ * ++ * * - * It is possible to recursively union union input gates. + * It is NOT possible to recursively union union input gates. --- End diff -- That would be a more invasive change in several places using `InputGate`s and we actually are able to use any other input gate types (if there were some) - just the recursive union does not make sense. So I guess, excluding ourself is the better approach here in terms of extensibility. Think of it as we're excluding this use not because we did not implement `pollNextBuffer` but because it does not make sense and we do not want this recursion. ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user NicoK commented on a diff in the pull request: https://github.com/apache/flink/pull/5583#discussion_r170987211 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/UnionInputGate.java --- @@ -189,11 +189,11 @@ public void requestPartitions() throws IOException, InterruptedException { bufferOrEvent.setChannelIndex(channelIndexOffset + bufferOrEvent.getChannelIndex()); - return Optional.ofNullable(bufferOrEvent); + return Optional.of(bufferOrEvent); } @Override - public Optional pollNextBufferOrEvent() throws IOException, InterruptedException { + public Optional pollNextBufferOrEvent() throws UnsupportedOperationException { --- End diff -- we don't need to - I thought this makes it more explicit if anyone tries to use this method in an IDE without looking at the code (which is a mistake imho, but commonly done) ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user pnowojski commented on a diff in the pull request: https://github.com/apache/flink/pull/5583#discussion_r170923061 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/UnionInputGate.java --- @@ -189,11 +189,11 @@ public void requestPartitions() throws IOException, InterruptedException { bufferOrEvent.setChannelIndex(channelIndexOffset + bufferOrEvent.getChannelIndex()); - return Optional.ofNullable(bufferOrEvent); + return Optional.of(bufferOrEvent); } @Override - public Optional pollNextBufferOrEvent() throws IOException, InterruptedException { + public Optional pollNextBufferOrEvent() throws UnsupportedOperationException { --- End diff -- Do you need to declare to throw this exception? Do we want to declare `RuntimeExceptions`? ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
Github user pnowojski commented on a diff in the pull request: https://github.com/apache/flink/pull/5583#discussion_r170923685 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/UnionInputGate.java --- @@ -61,7 +61,7 @@ * ++ * * - * It is possible to recursively union union input gates. + * It is NOT possible to recursively union union input gates. --- End diff -- I was thinking whether this class should take `SingleInputGate`s in the constructor instead of `InputGate`s and thus we could remove `pollNextBufferOrEvent` method from the interface and move it to the `SingleInputGate` only. ---
[GitHub] flink pull request #5583: [FLINK-8737][network] disallow creating a union of...
GitHub user NicoK opened a pull request: https://github.com/apache/flink/pull/5583 [FLINK-8737][network] disallow creating a union of UnionGate instances ## What is the purpose of the change Recently, the `InputGate#pollNextBufferOrEvent()` was added but not implemented in `UnionInputGate`. It is, however, used in `UnionInputGate#getNextBufferOrEvent()` and thus any `UnionInputGate` containing a `UnionInputGate` would have failed already. There should be no use case for wiring up inputs this way. Therefore, fail early when trying to construct this. ## Brief change log - throw an `UnsupportedOperationException` when trying to use a `UnionInputGate` for input to a `UnionInputGate` ## Verifying this change This change is a trivial rework / code cleanup without any test coverage. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): **no** - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: **no** - The serializers: **no** - The runtime per-record code paths (performance sensitive): **no** - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: **no** - The S3 file system connector: **no** ## Documentation - Does this pull request introduce a new feature? **no** - If yes, how is the feature documented? **JavaDocs** You can merge this pull request into a Git repository by running: $ git pull https://github.com/NicoK/flink flink-8737 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/5583.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #5583 commit 198bf145e2a4b81327ae0c1e58c5bf4f8a5da650 Author: Nico KruberDate: 2018-02-26T16:50:10Z [hotfix][network] minor improvements in UnionInputGate commit 6eff3e396b5a98a1029e3f42b5e78abfb10da7a6 Author: Nico Kruber Date: 2018-02-26T16:52:37Z [FLINK-8737][network] disallow creating a union of UnionInputGate instances Recently, the pollNextBufferOrEvent() was added but not implemented but this is used in getNextBufferOrEvent() and thus any UnionInputGate containing a UnionInputGate would have failed already. There should be no use case for wiring up inputs this way. Therefore, fail early when trying to construct this. ---