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

Revise Datastream to BigQuery IT's #1644

Merged

Conversation

Polber
Copy link
Contributor

@Polber Polber commented Jun 7, 2024

No description provided.

@Polber Polber self-assigned this Jun 7, 2024
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 42.26%. Comparing base (e19dcbc) to head (2507001).
Report is 7 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1644      +/-   ##
============================================
- Coverage     42.27%   42.26%   -0.01%     
+ Complexity     3167     3166       -1     
============================================
  Files           791      791              
  Lines         46131    46137       +6     
  Branches       4934     4935       +1     
============================================
- Hits          19502    19501       -1     
- Misses        25039    25045       +6     
- Partials       1590     1591       +1     
Components Coverage Δ
spanner-templates 63.63% <ø> (-0.01%) ⬇️
spanner-import-export 64.42% <ø> (-0.03%) ⬇️
spanner-live-forward-migration 74.97% <ø> (ø)
spanner-live-reverse-replication 51.44% <ø> (ø)
spanner-bulk-migration 83.14% <ø> (ø)
Files Coverage Δ
.../it/gcp/bigquery/conditions/BigQueryRowsCheck.java 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@Polber Polber force-pushed the jkinard/datastream-bq branch 5 times, most recently from fb7d28d to 2d025b3 Compare June 14, 2024 23:27
@Polber Polber force-pushed the jkinard/datastream-bq branch 7 times, most recently from f85ad56 to 1492a6e Compare June 20, 2024 20:45
@Polber Polber changed the title Jkinard/datastream bq Revise Datastream to BigQuery IT's Jun 25, 2024
@Polber Polber marked this pull request as ready for review June 25, 2024 17:35
@Polber Polber force-pushed the jkinard/datastream-bq branch 2 times, most recently from 8ca570b to 8682555 Compare June 26, 2024 16:50
@Polber Polber force-pushed the jkinard/datastream-bq branch 2 times, most recently from ccc500a to 641eb15 Compare July 11, 2024 02:09
@Polber Polber requested a review from damccorm July 11, 2024 17:11
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just one minor suggestion

try {
return resourceManager().getRowCount(tableId().getTable());
} catch (Exception e) {
if (ExceptionUtils.containsMessage(e, "Not found: Table")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems kind of odd to handle this exception at this level, especially since there's only one caller who does conditional logic on top anyways. Could we more cleanly just handle this in check instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that is much cleaner - thanks

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>
@damccorm damccorm added the Google LGTM Approval of a pull request to be merged into the repository label Jul 12, 2024
@copybara-service copybara-service bot merged commit 7662362 into GoogleCloudPlatform:main Jul 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Google LGTM Approval of a pull request to be merged into the repository size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants