[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user NicoK commented on the issue: https://github.com/apache/flink/pull/4552 looks good - Let's start some cluster tests and then we're ready to merge ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 1. Thanks for you FLINK08425. 2. I would have thought the tests for `ResultSubpartition#nextBufferIsEvent` which have already been covered before. The test for `BufferAndBacklog#nextBufferIsEvent()` is not included before, and thanks for providing the patch for it. 3. I will create a separate JIRA for the switch and also include the commit in this PR. And check the travis fail. 4. I will add the comment for the document you mentioned later. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @NicoK , I have submitted the switch for keeping the old mode and the new credit-based mode. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @NicoK , thanks for your reviews! I have submitted all the patches you provided offline to address above issues. 1. Remove `FLINK-8425` from this PR. 2. Do you think I should add more tests for `nextBufferIsEvent`? Because I already verified that in previous related tests 3. For adding the switch issue, I found some difficulties to leave messages for you offline. We can further confirm that. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user NicoK commented on the issue: https://github.com/apache/flink/pull/4552 one thing which we talked about offline: as a precaution, we should keep the old implementation around and allow the users to basically turn the credit-based flow control algorithm on/off (the accounting for the credits would mostly stay in that case but will simple not be used by the old non-existing flow control) ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @NicoK , I have submitted all the modifications based on the patches you provided. The tests for `nextBufferIsEvent` will be added in a new commit tomorrow. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @NicoK , I have rebased the latest codes. Wish your reviews! ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 Yes, I am planing to rebase this with the latest changes of previous commits. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user NicoK commented on the issue: https://github.com/apache/flink/pull/4552 Ok, I think, I'll manage the review without the split. Since this is the last of the credit-based PRs though, can you rebase on top of the latest changes (preferably after addressing the comments in the other PRs)? This way, I'll have the full picture of this crucial change. ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @NicoK , thanks for focusing on the last PR. I am supposed to divide this into two separate ones as you said. But I am afraid some current tests may fail if I only modify and enable the credit-based process on sender side, otherwise I may need to create a temporary handler in parallel with `PartitionRequestQueue`. As you said, the latter mainly replaces the `PartitionRequestClientHandler` with the contents of `CreditBasedClientHandler`, also it replaces the `CreditBasedClientHandler` class name in previous added tests and remove previous temporary codes in `ResultPartitionType` to make fixed buffer pool work. If it still brings difficulty for your review, then I will try to divide it then. : ) ---
[GitHub] flink issue #4552: [FLINK-7456][network] Implement Netty sender incoming pip...
Github user zhijiangW commented on the issue: https://github.com/apache/flink/pull/4552 @pnowojski , this PR is ready for review. It covers almost all the logics of credit-based on sender side. In addition, I replace the current `PartitionRequestClientHandler` with `CreditBasedClientHandler` and remove previous temporary codes for making this feature work on both sides. It leaves a small work to do in this PR related with `SpilledSubpartitionView#nextBufferIsEvent` because the existing process in spilled sub-partition can not get next buffer directly. But the current default value for `nextBufferIsEvent`` will not affect the core process, only results in wasting a unnecessary credit, then I will try to solve it in a lightweight way later. ---
