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

test: add data compatibility test #3109

Merged

Conversation

MichaelScofield
Copy link
Collaborator

@MichaelScofield MichaelScofield commented Jan 5, 2024

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Add a test for data files compatibility. See the readme for more details.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

#1730

@github-actions github-actions bot added docs-required This change requires docs update. Size: M labels Jan 5, 2024
@github-actions github-actions bot added docs-not-required This change does not impact docs. and removed docs-required This change requires docs update. labels Jan 5, 2024
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (50d16d6) 85.65% compared to head (939355d) 84.80%.
Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3109      +/-   ##
==========================================
- Coverage   85.65%   84.80%   -0.86%     
==========================================
  Files         859      880      +21     
  Lines      140757   143115    +2358     
==========================================
+ Hits       120571   121374     +803     
- Misses      20186    21741    +1555     

@sunng87
Copy link
Member

sunng87 commented Jan 6, 2024

I'm not a big fan of checking-in blobs. Can we use a prebuilt docker image for that?

Checking-in blobs will increase the repo size and make clone and fetch slow. We will be running these tests for every future version, which means more blobs to be added to repo. I'm afraid this approach won't be scalable.

@MichaelScofield
Copy link
Collaborator Author

@sunng87 Those binary files are not large for now, only 2.8MB compared to total repo size 76MB. We can keep this size small.

Besides, there always should be only one piece of data that are tested against the newly built greptimedb. Should there be any breaking changes, the data need to be generated again (using current codes).

Finally, a pre-built image for this simple purpose is kind of overkill. To make it worse, the downloading and running of the image will increase the github CI time a lot.

So I think at this early stage, let's keep it simple and straightforward. If there're more complex compatibility testing cases, we consider the other way.

@waynexia
Copy link
Member

waynexia commented Jan 9, 2024

I'm negative about this either. Those files are likely to change from version to version, I prefer to track those fixtures elsewhere, like a pre-build image or a separate repo like https://github.com/apache/parquet-testing, where

  • Updates to fixtures are aggregated
  • Won't fold the code repo
  • It's easy to maintain different versions and formats

@MichaelScofield
Copy link
Collaborator Author

I think running compatibility test in github CI is a must, it tells us which PR has breaking change immediately. Another repo or pre-built image are overkill for this, complicate things a lot. Not to metion the time cost. I'm planning refer to how databend does the compatibility test (see https://github.com/datafuselabs/databend/tree/main/tests/fuse-compat), that:

  1. download the latest binary from github release(the "old" version)
  2. build a binary from current source(the "new" version)
  3. start the old binary, insert some data into it, kill
  4. start the new binary, set its data home to the old, query some data to verify the compatibility

WDYT @sunng87 @waynexia @killme2008 @fengjiachun

@MichaelScofield MichaelScofield marked this pull request as draft January 9, 2024 06:31
auto-merge was automatically disabled January 9, 2024 06:31

Pull request was converted to draft

@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch from cd9b9f7 to 2740c6b Compare January 9, 2024 07:04
@waynexia
Copy link
Member

waynexia commented Jan 9, 2024

This looks good to me. Generating data on testing can make this test more flexible, by this way we have the ability to check the compatibility between two arbitrary versions.

@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch 3 times, most recently from 6d22d86 to ffec25f Compare January 11, 2024 03:36
@sunng87
Copy link
Member

sunng87 commented Jan 11, 2024

Sounds ok to me. We can start from this approach and see if it scales

@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch 5 times, most recently from 847400b to 1668cf6 Compare January 18, 2024 03:00
@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch 3 times, most recently from 3f490be to 8099727 Compare January 25, 2024 08:01
@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch 3 times, most recently from 966147a to 156e7bc Compare January 30, 2024 07:42
@MichaelScofield MichaelScofield force-pushed the test/add-compatibility-test branch from 156e7bc to 329df4c Compare January 31, 2024 07:04
@github-actions github-actions bot added Size: L and removed Size: M labels Feb 2, 2024
@MichaelScofield MichaelScofield changed the title test: add compatibility test (part 1) test: add data compatibility test Feb 2, 2024
@MichaelScofield MichaelScofield marked this pull request as ready for review February 2, 2024 07:57
@github-actions github-actions bot added Size: M and removed Size: L labels Feb 2, 2024
@MichaelScofield
Copy link
Collaborator Author

@sunng87 @waynexia PTAL

Copy link
Member

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

other parts have been looking good to me

@MichaelScofield
Copy link
Collaborator Author

@waynexia PTAL

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

.github/workflows/develop.yml Outdated Show resolved Hide resolved
tests/compat/README.md Show resolved Hide resolved
tests/compat/util.sh Outdated Show resolved Hide resolved
@waynexia waynexia enabled auto-merge February 20, 2024 07:34
@waynexia waynexia added this pull request to the merge queue Feb 20, 2024
Merged via the queue into GreptimeTeam:main with commit eded088 Feb 20, 2024
23 checks passed
@MichaelScofield MichaelScofield deleted the test/add-compatibility-test branch February 21, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants