Skip to content
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

fix: change the encoding of actor mapping in grpc #2031

Merged
merged 11 commits into from
Apr 22, 2022

Conversation

xx01cyx
Copy link
Contributor

@xx01cyx xx01cyx commented Apr 21, 2022

What's changed and what's your intention?

  • Summarize your change
    • Change the encoding of actor mapping in grpc. Compress the data to avoid a too-long grpc message.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

#1443

@xx01cyx xx01cyx requested a review from skyzh April 21, 2022 09:23
@github-actions github-actions bot added the type/fix Bug fix label Apr 21, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 735 files.

Valid Invalid Ignored Fixed
732 1 2 0
Click to see the invalid file list
  • src/common/src/util/compress.rs

src/common/src/util/compress.rs Show resolved Hide resolved
xx01cyx and others added 3 commits April 21, 2022 17:25
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

src/stream/src/task/stream_manager.rs Outdated Show resolved Hide resolved
src/common/src/util/compress.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd prefer to store offsets in the mapping, so that things might be clearer, and follows most of the developers' conventions. e.g., if we have:

1, 1, 1, 2, 2, 2, 3, 3, 3,
then we store 0, 3, 6 in offsets, and 1, 2, 3 in data.

@BugenZhao
Copy link
Member

LGTM, but I'd prefer to store offsets in the mapping, so that things might be clearer, and follows most of the developers' conventions. e.g., if we have:

1, 1, 1, 2, 2, 2, 3, 3, 3, then we store 0, 3, 6 in offsets, and 1, 2, 3 in data.

Then we need another field for the length. 😁

@skyzh
Copy link
Contributor

skyzh commented Apr 21, 2022

LGTM, but I'd prefer to store offsets in the mapping, so that things might be clearer, and follows most of the developers' conventions. e.g., if we have:

1, 1, 1, 2, 2, 2, 3, 3, 3, then we store 0, 3, 6 in offsets, and 1, 2, 3 in data.

Then we need another field for the length. 😁

We can record n+1 offsets 🤪

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2031 (6f0f13c) into main (f9e486e) will increase coverage by 0.30%.
The diff coverage is 89.70%.

@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
+ Coverage   70.64%   70.95%   +0.30%     
==========================================
  Files         629      632       +3     
  Lines       80959    81082     +123     
==========================================
+ Hits        57195    57529     +334     
+ Misses      23764    23553     -211     
Flag Coverage Δ
rust 70.95% <89.70%> (+0.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/common/src/util/mod.rs 0.00% <ø> (ø)
src/meta/src/stream/stream_manager.rs 69.55% <0.00%> (-0.27%) ⬇️
src/stream/src/task/stream_manager.rs 0.00% <0.00%> (ø)
src/common/src/util/compress.rs 100.00% <100.00%> (ø)
.../frontend/src/optimizer/plan_node/logical_limit.rs 41.66% <0.00%> (-15.00%) ⬇️
src/stream/src/executor_v2/v1_compat.rs 30.56% <0.00%> (-2.23%) ⬇️
src/frontend/src/binder/expr/function.rs 89.58% <0.00%> (-1.95%) ⬇️
...ontend/src/optimizer/plan_node/stream_hash_join.rs 60.00% <0.00%> (-0.61%) ⬇️
src/meta/src/stream/graph/stream_graph.rs 59.95% <0.00%> (-0.56%) ⬇️
src/frontend/src/expr/function_call.rs 95.16% <0.00%> (-0.26%) ⬇️
... and 47 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@xx01cyx
Copy link
Contributor Author

xx01cyx commented Apr 21, 2022

We can record n+1 offsets 🤪

Then there will be little difference compared with current encoding 🤣

@skyzh
Copy link
Contributor

skyzh commented Apr 21, 2022

Guess 65536 is still too much to process, it seems that e2e timed out?

@xx01cyx
Copy link
Contributor Author

xx01cyx commented Apr 22, 2022

Guess 65536 is still too much to process, it seems that e2e timed out?

Might find out the reason later 🤣

@xx01cyx xx01cyx merged commit abb873a into main Apr 22, 2022
@xx01cyx xx01cyx deleted the fix/dispatcher-grpc-encoding branch April 22, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants