Skip to content

[BEAM-6627] Add size reporting to JdbcIOIT#10267

Merged
lgajowy merged 2 commits intoapache:masterfrom
mwalenia:BEAM-6627-jdbcioit-size
Dec 5, 2019
Merged

[BEAM-6627] Add size reporting to JdbcIOIT#10267
lgajowy merged 2 commits intoapache:masterfrom
mwalenia:BEAM-6627-jdbcioit-size

Conversation

@mwalenia
Copy link
Member

@mwalenia mwalenia commented Dec 3, 2019

I added size checking to JdbcIOIT only for PostgreSQL because the test is based on this dialect and I don't think adding more makes sense.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@mwalenia
Copy link
Member Author

mwalenia commented Dec 3, 2019

R: @lgajowy wdyt?

@mwalenia mwalenia force-pushed the BEAM-6627-jdbcioit-size branch from e8c6858 to 7e2c1f0 Compare December 3, 2019 09:55
Copy link
Contributor

@lgajowy lgajowy left a comment

Choose a reason for hiding this comment

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

Suggested a different approach. Let me know what do you think. Thanks!

statement.executeQuery(String.format("select pg_relation_size('%s')", tableName));
if (resultSet.next()) {
return resultSet.getLong(1);
} else return 0L;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks that the code returns 0L in case the size is not available (am I right?) Maybe we shouldn't return 0L in that case - "else" means in this case that "we couldn't retrieve the size" not that it is equal to 0.

Have you considered using Optional here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, that's a huge improvement. Thanks for the suggestion!

DatabaseTestHelper.getPostgresTableSize(dataSource, tableName);
return NamedTestResult.create(
uuid, timestamp, "total_size", tableSizeAfterWrite - tableSize);
} catch (SQLException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case optional could be useful too. The getPostgresTableSize could catch the error and return empty() in case of problems. Then, here, you could decide if to publish the metric or not, or even fail the whole test, depending on whether you have all the necessary metrics collected.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I changed it

@mwalenia
Copy link
Member Author

mwalenia commented Dec 4, 2019

@lgajowy WDYT now? I changed the methods to use Optional - it really helped, thanks!

@lgajowy
Copy link
Contributor

lgajowy commented Dec 4, 2019

Run Java JdbcIO Performance Test

Copy link
Contributor

@lgajowy lgajowy left a comment

Choose a reason for hiding this comment

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

LGTM

@lgajowy lgajowy merged commit a991aee into apache:master Dec 5, 2019
@mwalenia mwalenia deleted the BEAM-6627-jdbcioit-size branch January 24, 2020 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants