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

[metasrv] feature: exchange protocol version with client #5645

Merged
merged 10 commits into from
May 29, 2022

Conversation

drmingdrmer
Copy link
Member

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

Summary

[metasrv] feature: exchange protocol version with client

Before transfer any data, MetaGrpcClient must ensure it is compatible
with the metasrv it is connecting to, in the handshake RPC.

Add compatibility cross test:

latest query -- latest metasrv
old    query -- latest metasrv
latest query -- old    metasrv

Features

  • Compatibility test: download test suite for query.

    When testing an old query with latest metasrv,
    download the test suite for that version of query instead of using
    the latest suite, which may not pass if new feature test is added
    into the latest suite but not yet impl-ed in the old query.

  • Compatibility test:

    Test the compatibility between:

    • The latest query and the latest metasrv.
    • The oldest compatible query with latest metasrv.
    • The latest query with oldest compatible metasrv.
  • Protocol version exchange when meta-client handshake with metasrv.

    meta-client(query) keeps track of the min-metasrv version it is compatible
    with.

    metasrv keeps track of the min-meta-client version it is compatible
    with.

    Client handshake check these versions to ensure compatibility.
    Otherwise an error is returned and no further data will be transferred.

  • Add admin CLI command --cmd ver.

    databend-query --cmd ver outputs its build version and the minimal
    compatible metasrv version, such as:

    version: 0.7.61-nightly
    min-compatible-metasrv-version: 0.7.59
    

    databend-meta --cmd ver outputs its build version and the minimal
    compatible meta-client(query) version, such as:

    version: 0.7.61-nightly
    min-compatible-client-version: 0.7.57
    

Refactors

  • Add a new error InvalidArgument to indicate protocol-error

  • MetaGrpcClient::try_new() does not need to be async.

  • Metasrv config does not check() if --cmd is non-empty:

    When cmd is specified, it does not mean to run it.

Docs

  • Add doc explain how to check versions and how to upgrade.

Changelog

  • New Feature

Related Issues

@vercel
Copy link

vercel bot commented May 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview May 29, 2022 at 2:09AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented May 28, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label May 28, 2022
@drmingdrmer drmingdrmer marked this pull request as ready for review May 28, 2022 07:29
Before transfer any data, `MetaGrpcClient` must ensure it is compatible
with the metasrv it is connecting to, in the handshake RPC.

Add compatibility cross test:

```
latest query -- latest metasrv
old    query -- latest metasrv
latest query -- old    metasrv
```

Features

- Compatibility test: download test suite for query.

  When testing an old query with latest metasrv,
  download the test suite for that version of query instead of using
  the latest suite, which may not pass if new feature test is added
  into the latest suite but not yet impl-ed in the old query.

- Compatibility test:

  Test the compatibility between:
  - The latest query and the latest metasrv.
  - The oldest compatible query with latest metasrv.
  - The latest query with oldest compatible  metasrv.

- Protocol version exchange when meta-client `handshake` with metasrv.

  meta-client(query) keeps track of the min-metasrv version it is compatible
  with.

  metasrv keeps track of the min-meta-client version it is compatible
  with.

  Client handshake check these versions to ensure compatibility.
  Otherwise an error is returned and no further data will be transferred.

- Add admin CLI command `--cmd ver`.

  `databend-query --cmd ver` outputs its build version and the minimal
  compatible metasrv version, such as:

  ```
  version: 0.7.61-nightly
  min-compatible-metasrv-version: 0.7.59
  ```

  `databend-meta --cmd ver` outputs its build version and the minimal
  compatible meta-client(query) version, such as:

  ```
  version: 0.7.61-nightly
  min-compatible-client-version: 0.7.57
  ```

Refactors

- Add a new error `InvalidArgument` to indicate protocol-error

- `MetaGrpcClient::try_new()` does not need to be `async`.

- Metasrv config does not `check()` if `--cmd` is non-empty:

  When `cmd` is specified, it does not mean to run it.

Docs

- Add doc explain how to check versions and how to upgrade.

---

- Fix: databendlabs#5627
- Fix: databendlabs#4948
@BohuTANG
Copy link
Member

Cool.
If we build from source with out git information, what happenes during metasrv and query?

@drmingdrmer
Copy link
Member Author

Cool. If we build from source with out git information, what happenes during metasrv and query?

The commit-version will be missing and the query won't be able to connect to metasrv.

@drmingdrmer
Copy link
Member Author

Cool. If we build from source with out git information, what happenes during metasrv and query?

BTW I've once considered using the version defined in Cargo.toml for building a release.

[package]
name = "databend-query"
version = "0.7.0"

E.g., if the version in Cargo is 0.7.0, then a nightly build version could be 0.7.0+20220528.

Then the git-tag triggered release action only is used for formal releases.

@BohuTANG
Copy link
Member

Cool. If we build from source with out git information, what happenes during metasrv and query?

The commit-version will be missing and the query won't be able to connect to metasrv.

I think this is OK.

@drmingdrmer
Copy link
Member Author

Cool. If we build from source with out git information, what happenes during metasrv and query?

The commit-version will be missing and the query won't be able to connect to metasrv.

I think this is OK.

Hmm... looks like CI failed due to this issue. In the CI it just checkout one last commit without any tags. The version can not be determined.

It can be worked around by using a special protocol-version == 0, which I do not like. 🤔

@BohuTANG
Copy link
Member

Cool. If we build from source with out git information, what happenes during metasrv and query?

The commit-version will be missing and the query won't be able to connect to metasrv.

I think this is OK.

Hmm... looks like CI failed due to this issue. In the CI it just checkout one last commit without any tags. The version can not be determined.

It can be worked around by using a special protocol-version == 0, which I do not like. 🤔

@everpcpc fixed it, may help here

@@ -15,6 +15,10 @@ runs:
- name: Maximize build space
uses: ./.github/actions/cleanup

- name: Rust setup
Copy link
Member

@everpcpc everpcpc May 28, 2022

Choose a reason for hiding this comment

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

dev_setup is only needed on macos.
on linux ci, setup_build_tool already makes it working.

Copy link
Member Author

Choose a reason for hiding this comment

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

It complains in test-compat.sh that wget is not found.
What did I do wrongly?

I was trying to run it with build-tool:
build-tool bash ./tests/compat/test-compat.sh

wget should be already provided by github-action.

Copy link
Member

@everpcpc everpcpc May 28, 2022

Choose a reason for hiding this comment

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

ah, it's not in the build-tool image now.
build-tool command will run command in container with image datafuselabs/build-tool:dev
maybe you can use curl instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's all right! Thanks!

@everpcpc
Copy link
Member

Cool. If we build from source with out git information, what happenes during metasrv and query?

The commit-version will be missing and the query won't be able to connect to metasrv.

I think this is OK.

Hmm... looks like CI failed due to this issue. In the CI it just checkout one last commit without any tags. The version can not be determined.
It can be worked around by using a special protocol-version == 0, which I do not like. 🤔

@everpcpc fixed it, may help here

should be fixed with: https://github.com/datafuselabs/databend/pull/5520/files#r884142502

@drmingdrmer
Copy link
Member Author

should be fixed with: https://github.com/datafuselabs/databend/pull/5520/files#r884142502

Is it appropriate to fetch the full history?
The fetching time would be longer then.

- name: Rust setup
run: |
bash ./scripts/setup/dev_setup.sh -yb

- name: Setup Build Tool
uses: ./.github/actions/setup_build_tool

Copy link
Member

Choose a reason for hiding this comment

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

installing python libs is also not need, and already done by build tool image

@everpcpc
Copy link
Member

should be fixed with: https://github.com/datafuselabs/databend/pull/5520/files#r884142502

Is it appropriate to fetch the full history? The fetching time would be longer then.

we are already doing this with release build, and the checkout takes around 10s which seems fine.

@mergify mergify bot merged commit 7cf4440 into databendlabs:main May 29, 2022
@drmingdrmer drmingdrmer deleted the 30-protocol-ver branch May 29, 2022 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
5 participants