Revamp check_format_compatible.sh #8012
Conversation
Summary: * Adds backup/restore forward/backward compatibility testing * Adds forward/backward compatibility testing to sst ingestion * More structure sharing and comments for the lists of branches comprising each group * Less reliant on invariants between groups with de-duplication logic * Restructured for n+1 branch checkout+build steps rather than something like 3n. Should be much faster despite more checks. And to make manual runs easier * On success, restores working trees to original working branch (aborts early if uncommitted changes) and deletes temporary branch & remote * Adds SHORT_TEST=1 mode that uses only the oldest version for each * Adds USE_SSH=1 to use ssh instead of https for github group Test Plan: a number of manual tests, mostly with SHORT_TEST=1. Using one version older for any of the groups (except I didn't check db_backward_only_refs) fails. Changing default format_version to 5 (planned) without updating this script fails as it should, and passes with appropriate update. TODO: full local run
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
LGTM with minor comments/questions. Thanks @pdillinger for the improvement!
tools/check_format_compatible.sh
Outdated
| } | ||
|
|
||
| # To restore to prior branch at the end | ||
| orig_branch=`git branch --show-current` |
There was a problem hiding this comment.
Nit: git branch --show-current is not supported on some versions.
tools/check_format_compatible.sh
Outdated
| # Don't depend on what current "origin" might be | ||
| git remote remove $tmp_origin 2>/dev/null | ||
| if [ "$USE_SSH" ]; then | ||
| git remote add $tmp_origin "git@github.com:facebook/rocksdb.git" |
There was a problem hiding this comment.
Can we make the url configurable so that it is easy for PR authors to run this test? The default url can still be official facebook repo of course.
There was a problem hiding this comment.
This remote is only used with the hard-coded branch names in the test. The passed on the command line can be any locally recognized ref, including HEAD.
tools/check_format_compatible.sh
Outdated
| echo "== Use $current_checkout_name to ingest extern SST file from $checkout_ref" | ||
| ingest_external_sst $ext_test_dir/${checkout_ref}_ingest $current_ext_test_dir | ||
| compare_db $ext_test_dir/${checkout_ref}_ingest ${current_ext_test_dir}_ingest db_dump.txt 1 1 | ||
| echo hi |
There was a problem hiding this comment.
Is this intended or a temporary debug message?
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pdillinger merged this pull request in a9046f3. |
|
Running time in CircleCI went from 2h13m to 59m. Before: https://app.circleci.com/pipelines/github/facebook/rocksdb/5990/workflows/62a1c39e-894e-4dfc-97e6-d4b7a2ac22ed/jobs/100162 |
Summary: * Adds backup/restore forward/backward compatibility testing * Adds forward/backward compatibility testing to sst ingestion * More structure sharing and comments for the lists of branches comprising each group * Less reliant on invariants between groups with de-duplication logic * Restructured for n+1 branch checkout+build steps rather than something like 3n. Should be much faster despite more checks. And to make manual runs easier * On success, restores working trees to original working branch (aborts early if uncommitted changes) and deletes temporary branch & remote * Adds SHORT_TEST=1 mode that uses only the oldest version for each * Adds USE_SSH=1 to use ssh instead of https for github group Pull Request resolved: facebook#8012 Test Plan: a number of manual tests, mostly with SHORT_TEST=1. Using one version older for any of the groups (except I didn't check db_backward_only_refs) fails. Changing default format_version to 5 (planned) without updating this script fails as it should, and passes with appropriate update. Full local run passed (had to remove "2.7.fb.branch" due to compiler issues, also before this change). Reviewed By: riversand963 Differential Revision: D26735840 Pulled By: pdillinger fbshipit-source-id: 1320c22de5674760657e385aa42df9fade8b6fff
Summary:
comprising each group
like 3n. Should be much faster despite more checks.
And to make manual runs easier
early if uncommitted changes) and deletes temporary branch & remote
group
Test Plan: a number of manual tests, mostly with SHORT_TEST=1. Using one
version older for any of the groups (except I didn't check
db_backward_only_refs) fails. Changing default format_version to 5
(planned) without updating this script fails as it should, and passes
with appropriate update. Full local run passed (had to remove "2.7.fb.branch"
due to compiler issues, also before this change).