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.

Sign up for GitHub

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

Jump to bottom

refactor: Align versions for arrow, opendal, hyper, tonic, prost #16704

Merged
merged 26 commits into from
Oct 29, 2024
Merged

refactor: Align versions for arrow, opendal, hyper, tonic, prost #16704

merged 26 commits into from
Oct 29, 2024

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Oct 28, 2024 •
edited by drmingdrmer
Loading

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

This PR will align most deps version to one, like arrow, opendal, hyper, tonic, prost and so on.

This is a huge PR and changes nearly all crates, please merge this PR first.

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@Xuanwo Xuanwo changed the title Align versions refactor(*): Align versions for arrow, opendal, hyper, tonic, prost and so on Oct 28, 2024
@Xuanwo Xuanwo changed the title refactor(*): Align versions for arrow, opendal, hyper, tonic, prost and so on refactor: Align versions for arrow, opendal, hyper, tonic, prost Oct 28, 2024
@github-actions github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Oct 28, 2024
drmingdrmer
drmingdrmer approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 47 of 47 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Xuanwo)

@Xuanwo Xuanwo mentioned this pull request Oct 28, 2024
Open
17 tasks
@Xuanwo
fix scacar
3485559 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix logging build
d536ffe 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
bump databend driver
a4efef6 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
format toml
d3a14ce 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Update parquet result
f5e4519 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix iceberg
5a02d22 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix tests
ed14197 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Polish deps
a75b5e2 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Merge branch 'main' into align-versions
d68f5e3
@Xuanwo
format toml
5cab05c 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Merge remote-tracking branch 'refs/remotes/xuanwo/align-versions' int…
8ccd8ac 
…o align-versions
@Xuanwo
Fix test
6036b5a 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix test
971c584 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix tests
40bfb99 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix tests
d03bea5 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 28, 2024

Only this test failed:

     Summary [ 238.419s] 1938 tests run: 1937 passed (2 slow), 1 failed, 14 skipped
        FAIL [   2.530s] databend-meta::it grpc::metasrv_grpc_kv_api_restart_cluster::test_kv_api_restart_cluster_token_expired

Not sure if related.

@everpcpc
Copy link
Member

everpcpc commented Oct 29, 2024 •
edited
Loading

Only this test failed:

     Summary [ 238.419s] 1938 tests run: 1937 passed (2 slow), 1 failed, 14 skipped
        FAIL [   2.530s] databend-meta::it grpc::metasrv_grpc_kv_api_restart_cluster::test_kv_api_restart_cluster_token_expired

Not sure if related.

It's a restart test, and the error is port in use.
Maybe some SO_REUSEADDR for the server changed to disabled after some upgrade?
I think it's not a big problem. We could skip and figure out it later?
cc @Xuanwo

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 29, 2024

It's a restart test, and the error is port in use.
Maybe some SO_REUSEADDR for the server changed to disabled after some upgrade?
I think it's not a big problem. We could skip and figure out it later?

I'm fine with skipping it. What do you think? Cc @drmingdrmer

@drmingdrmer
Copy link
Member

It's a restart test, and the error is port in use.
Maybe some SO_REUSEADDR for the server changed to disabled after some upgrade?
I think it's not a big problem. We could skip and figure out it later?

I'm fine with skipping it. What do you think? Cc @drmingdrmer

I'm did not yet figure out the cause of the failure.
This test case ensures meta-client auto-reconnect if the target meta-server is shutdown.
The only risk is auto-reconnect may not be provided by next build.

Is it acceptable? @everpcpc

@everpcpc
Copy link
Member

everpcpc commented Oct 29, 2024 •
edited
Loading

It's a restart test, and the error is port in use.
Maybe some SO_REUSEADDR for the server changed to disabled after some upgrade?
I think it's not a big problem. We could skip and figure out it later?

I'm fine with skipping it. What do you think? Cc @drmingdrmer

I'm did not yet figure out the cause of the failure. This test case ensures meta-client auto-reconnect if the target meta-server is shutdown. The only risk is auto-reconnect may not be provided by next build.

Is it acceptable? @everpcpc

It's not, but auto-reconnect may not be affected, and we could fix the test later. We do not upgrade meta frequently.

@drmingdrmer
Copy link
Member

It's not, but auto-reconnect maybe not affected, and we could fix the test later. We do not upgrade meta frequently.

Ok let's merge

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 29, 2024

Got it, I will merge with main and skip this test.

@Xuanwo Xuanwo enabled auto-merge October 29, 2024 09:44
@Xuanwo Xuanwo added this pull request to the merge queue Oct 29, 2024
Xuanwo
Xuanwo commented Oct 29, 2024
View reviewed changes
Copy link
Member Author

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Reviewed 43 of 47 files at r1, 4 of 6 files at r2, 4 of 6 files at r3, 13 of 13 files at r4, 5 of 5 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Xuanwo)

Merged via the queue into databendlabs:main with commit c44ea20 Oct 29, 2024
73 checks passed
@Xuanwo Xuanwo deleted the align-versions branch October 29, 2024 10:51
drmingdrmer added a commit to drmingdrmer/databend that referenced this pull request Oct 30, 2024
@drmingdrmer
refactor: MetaGrpcClient needs to specify timeout to auto-reconnect t…
a7e250f 
…o restarted server

Before c44ea20, MetaGrpcClient just
auto reconnect if the target meta-service server is restarted.

After c44ea20, the client behavior
changed, if no timeout is set, the client reconnect to the restarted
server but it hangs forever.

In this commit, in the test: test_kv_api_restart_cluster_token_expired,
give the client a fixed timeout to survive a server restart.

- Fix: databendlabs#16704 (comment)
@drmingdrmer drmingdrmer mentioned this pull request Oct 30, 2024
Merged
5 tasks
drmingdrmer added a commit to drmingdrmer/databend that referenced this pull request Oct 30, 2024
@drmingdrmer
refactor: MetaGrpcClient needs to specify timeout to auto-reconnect t…
4246e15 
…o restarted server

Before c44ea20, MetaGrpcClient just
auto reconnect if the target meta-service server is restarted.

After c44ea20, the client behavior
changed, if no timeout is set, the client reconnect to the restarted
server but it hangs forever.

In this commit, in the test: test_kv_api_restart_cluster_token_expired,
give the client a fixed timeout to survive a server restart.

- Fix: databendlabs#16704 (comment)
drmingdrmer added a commit that referenced this pull request Oct 30, 2024
@drmingdrmer
refactor: MetaGrpcClient needs to specify timeout to auto-reconnect t…
0bd9509 
…o restarted server (#16733)

Before c44ea20, MetaGrpcClient just
auto reconnect if the target meta-service server is restarted.

After c44ea20, the client behavior
changed, if no timeout is set, the client reconnect to the restarted
server but it hangs forever.

In this commit, in the test: test_kv_api_restart_cluster_token_expired,
give the client a fixed timeout to survive a server restart.

- Fix: #16704 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drmingdrmer drmingdrmer drmingdrmer approved these changes

Assignees
No one assigned
Labels
pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xuanwo @everpcpc @drmingdrmer