Skip to content

feat(test): Add double migration test #827

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

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

candiduslynx
Copy link
Contributor

We saw some bugs when the migration code wasn't reliable and, if we ran safe migration after forced one, there would be conflicts reported (where they shouldn't have been).
This extra test ensures that the destination is in sync what it scans from database and what is taken from spec.

Copy link
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

can we just add it to an already existing force test?

@github-actions
Copy link

github-actions bot commented Apr 27, 2023

⏱️ Benchmark results

Comparing with f32fac3

  • DefaultConcurrencyDFS-2 resources/s: 11,324 ⬇️ 1.33% decrease vs. f32fac3
  • DefaultConcurrencyRoundRobin-2 resources/s: 11,399 ⬇️ 7.26% decrease vs. f32fac3
  • Glob-2 ns/op: 179.4 ⬇️ 25.25% decrease vs. f32fac3
  • TablesWithChildrenDFS-2 resources/s: 29,030 ⬆️ 10.85% increase vs. f32fac3
  • TablesWithChildrenRoundRobin-2 resources/s: 28,914 ⬆️ 9.73% increase vs. f32fac3
  • TablesWithRateLimitingDFS-2 resources/s: 28.22 ⬇️ 0.35% decrease vs. f32fac3
  • TablesWithRateLimitingRoundRobin-2 resources/s: 830.7 ⬆️ 3.01% increase vs. f32fac3
  • BufferedScanner-2 ns/op: 9.337 ⬇️ 11.71% decrease vs. f32fac3
  • LogReader-2 ns/op: 30.51 ⬇️ 9.80% decrease vs. f32fac3

@candiduslynx
Copy link
Contributor Author

candiduslynx commented Apr 27, 2023

can we just add it to an already existing force test?

I think this should be done here as we want to ensure the full consistency in a simple way.
We would have to do a separate test per write mode, but here it's a relatively small change)

@candiduslynx candiduslynx force-pushed the feat/migrate-normal-after-force-test branch from bda7bc1 to 0704777 Compare April 27, 2023 15:26
@hermanschaaf
Copy link
Member

I don't mind where it's done, as long as it is done; this test is a great addition to the test suite imo 👍

@candiduslynx candiduslynx merged commit 4cd3872 into main Apr 27, 2023
@candiduslynx candiduslynx deleted the feat/migrate-normal-after-force-test branch April 27, 2023 18:02
candiduslynx pushed a commit that referenced this pull request Apr 28, 2023
🤖 I have created a release *beep* *boop*
---


##
[2.5.0](v2.4.0...v2.5.0)
(2023-04-28)


### Features

* Add table description to Arrow schema metadata
([#824](#824))
([1a8072f](1a8072f))
* **arrow:** Streamline Apache Arrow extension types
([#823](#823))
([f32fac3](f32fac3))
* **test:** Add double migration test
([#827](#827))
([4cd3872](4cd3872))
* Time values are truncated uniformly
([#825](#825))
([ffb97b0](ffb97b0))


### Bug Fixes

* TransformWithStruct/DefaultNameTransformer change for invalid column
names ([#820](#820))
([01e6649](01e6649))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants