Skip to content

[CELEBORN-770][FLINK] Convert BacklogAnnouncement, BufferStreamEnd, ReadAddCredit to PB #1905

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

Closed
wants to merge 1 commit into from

Conversation

SteNicholas
Copy link
Member

What changes were proposed in this pull request?

BacklogAnnouncement, BufferStreamEnd, and ReadAddCredit should merge to transport messages to enhance celeborn's compatibility.

Why are the changes needed?

  1. Improves celeborn's transport flexibility to change RPC.
  2. Makes Compatible with 0.2 client.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • TransportFrameDecoderWithBufferSupplierSuiteJ

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #1905 (903dce4) into main (55e8505) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1905      +/-   ##
==========================================
- Coverage   46.66%   46.59%   -0.07%     
==========================================
  Files         164      164              
  Lines       10283    10293      +10     
  Branches      938      938              
==========================================
- Hits         4798     4795       -3     
- Misses       5171     5184      +13     
  Partials      314      314              
Files Changed Coverage Δ
...eleborn/common/network/client/TransportClient.java 39.86% <0.00%> (-1.49%) ⬇️
...n/common/network/protocol/BacklogAnnouncement.java 0.00% <0.00%> (ø)
...eborn/common/network/protocol/BufferStreamEnd.java 0.00% <0.00%> (-30.00%) ⬇️
...eleborn/common/network/protocol/ReadAddCredit.java 0.00% <ø> (ø)
...born/common/network/protocol/TransportMessage.java 60.00% <0.00%> (-5.62%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SteNicholas SteNicholas reopened this Sep 13, 2023
@FMX
Copy link
Contributor

FMX commented Sep 13, 2023

2023-09-13T11:41:04.1056748Z 23/09/13 11:41:04,085 ERROR [data-client-50-1] ReadClientHandler: Unexpected msg type RPC_REQUEST content RpcRequest{requestId=268, body=NettyManagedBuffer{buf=UnpooledSlicedByteBuf(ridx: 0, widx: 17, cap: 17/17, unwrapped: UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeNoCleanerDirectByteBuf(ridx: 89, widx: 89, cap: 1024))}}

ReadClientHandler needs modifications to receive rpc requests. Sizable CI failures mean that something is wrong.

@SteNicholas SteNicholas changed the title [CELEBORN-770][FLINK] Convert BacklogAnnouncement,BufferStreamEnd,ReadAddCredit to PB [CELEBORN-770][FLINK] Convert BacklogAnnouncement, BufferStreamEnd, ReadAddCredit to PB Sep 13, 2023
Copy link
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadClientHandler needs modification.

@RexXiong RexXiong closed this in 2407cae Sep 25, 2023
RexXiong pushed a commit that referenced this pull request Sep 25, 2023
…eadAddCredit to PB

`BacklogAnnouncement`, `BufferStreamEnd`, and `ReadAddCredit` should merge to transport messages to enhance celeborn's compatibility.

1. Improves celeborn's transport flexibility to change RPC.
2. Makes Compatible with 0.2 client.

No.

- `TransportFrameDecoderWithBufferSupplierSuiteJ`

Closes #1905 from SteNicholas/CELEBORN-770.

Authored-by: SteNicholas <[email protected]>
Signed-off-by: Shuang <[email protected]>
(cherry picked from commit 2407cae)
Signed-off-by: Shuang <[email protected]>
@RexXiong
Copy link
Contributor

Thanks merge to main/0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants