Skip to content

Conversation

@yevgenypats
Copy link
Contributor

A bit more footgun in old protocol.

@yevgenypats yevgenypats requested a review from hermanschaaf May 16, 2023 13:59
@github-actions github-actions bot added the fix label May 16, 2023
return nil, status.Errorf(codes.InvalidArgument, "failed to unmarshal tables: %v", err)
}
tables := TablesV2ToV3(tablesV2)
tables := TablesV2ToV3(tablesV2).FlattenTables()
Copy link
Member

Choose a reason for hiding this comment

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

If this is the destination server, presumably it got the list from the source - so shouldn't the tables already be flattened? I know there can be issues if you flatten a list of tables twice, so we either need to be 100% sure or make FlattenTables idempotent :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. yeah that's a foot gun. not sure what's hapening already. ok let's make it idempotent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (7d749b1) 47.29% compared to head (ba49bc4) 47.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
+ Coverage   47.29%   47.37%   +0.07%     
==========================================
  Files          55       55              
  Lines        5034     5041       +7     
==========================================
+ Hits         2381     2388       +7     
  Misses       2401     2401              
  Partials      252      252              
Impacted Files Coverage Δ
schema/table.go 35.31% <100.00%> (+1.52%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented May 16, 2023

⏱️ Benchmark results

Comparing with 7d749b1

  • DefaultConcurrencyDFS-2 resources/s: 10,322 ⬇️ 1.19% decrease vs. 7d749b1
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,277 ⬇️ 1.81% decrease vs. 7d749b1
  • Glob-2 ns/op: 288.9 ⬆️ 18.07% increase vs. 7d749b1
  • TablesWithChildrenDFS-2 resources/s: 24,018 ⬇️ 9.61% decrease vs. 7d749b1
  • TablesWithChildrenRoundRobin-2 resources/s: 25,137 ⬇️ 4.50% decrease vs. 7d749b1
  • TablesWithRateLimitingDFS-2 resources/s: 28.48 ⬆️ 0.49% increase vs. 7d749b1
  • TablesWithRateLimitingRoundRobin-2 resources/s: 807.4 ⬆️ 0.30% increase vs. 7d749b1

@yevgenypats yevgenypats requested a review from hermanschaaf May 16, 2023 14:32
@kodiakhq kodiakhq bot merged commit 28706f1 into main May 16, 2023
@kodiakhq kodiakhq bot deleted the fix/flatten_v2 branch May 16, 2023 14:46
kodiakhq bot pushed a commit that referenced this pull request May 16, 2023
🤖 I have created a release *beep* *boop*
---


## [3.5.1](v3.5.0...v3.5.1) (2023-05-16)


### Bug Fixes

* Flatten V2 tables ([#882](#882)) ([28706f1](28706f1))

---
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