Add support for parallel exports #52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PTAL @labianchin |
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
========================================
Coverage ? 88.9%
Complexity ? 165
========================================
Files ? 21
Lines ? 622
Branches ? 43
========================================
Hits ? 553
Misses ? 47
Partials ? 22 |
labianchin
reviewed
Mar 8, 2019
| return Lists.newArrayList( | ||
| String.format("SELECT * FROM %s%s%s", this.tableName(), where, limit)); | ||
|
|
||
| if (parallelism().isPresent() && splitColumn().isPresent()) { |
There was a problem hiding this comment.
Maybe extract this if branch into separate method?
labianchin
reviewed
Mar 18, 2019
| final ResultSet | ||
| resultSet = | ||
| statement.executeQuery(query); | ||
| resultSet.first(); |
There was a problem hiding this comment.
[main] INFO com.spotify.dbeam.avro.BeamJdbcAvroSchema - Elapsed time to schema 0.452 seconds
[main] ERROR com.spotify.dbeam.jobs.ExceptionHandling - Failure:
org.postgresql.util.PSQLException: Operation requires a scrollable ResultSet, but this ResultSet is FORWARD_ONLY.
at org.postgresql.jdbc.PgResultSet.checkScrollable(PgResultSet.java:280)
at org.postgresql.jdbc.PgResultSet.first(PgResultSet.java:355)
at com.spotify.dbeam.args.QueryBuilderArgs.findSplitLimits(QueryBuilderArgs.java:178)
at com.spotify.dbeam.args.QueryBuilderArgs.buildQueries(QueryBuilderArgs.java:139)
at com.spotify.dbeam.jobs.JdbcAvroJob.prepareExport(JdbcAvroJob.java:93)
at com.spotify.dbeam.jobs.JdbcAvroJob.runExport(JdbcAvroJob.java:134)
at com.spotify.dbeam.jobs.JdbcAvroJob.main(JdbcAvroJob.java:142)
Maybe use checkState(resultSet.next(), "Min/Max query returned empty results"); instead.
There was a problem hiding this comment.
Good catch.. I'll add a check
|
@labianchin this is now ready for merging. I would update the documentation and add best practices separately. |
|
Let's ship this, try a bit and then iterate a bit more on it. We might still be missing metrics and maybe some tests. But given this is a new feature and it does not break existing flows, we can ship and try. |
labianchin
pushed a commit
that referenced
this pull request
Apr 16, 2019
* add draft support for parallel exports * separate functions for split queries * fix doc * fix logic and add test cases * add check for -ve parallelism * refactor metering for parallel exports * check result set * rename to query parallelism to not conflict with beam's targetParallelism * fix record gauge reporting * change order of e2e test * fix metering for parallel queries * add e2e test for parallel query
labianchin
pushed a commit
that referenced
this pull request
Apr 16, 2019
* add draft support for parallel exports * separate functions for split queries * fix doc * fix logic and add test cases * add check for -ve parallelism * refactor metering for parallel exports * check result set * rename to query parallelism to not conflict with beam's targetParallelism * fix record gauge reporting * change order of e2e test * fix metering for parallel queries * add e2e test for parallel query
labianchin
pushed a commit
that referenced
this pull request
Apr 16, 2019
* add draft support for parallel exports * separate functions for split queries * fix doc * fix logic and add test cases * add check for -ve parallelism * refactor metering for parallel exports * check result set * rename to query parallelism to not conflict with beam's targetParallelism * fix record gauge reporting * change order of e2e test * fix metering for parallel queries * add e2e test for parallel query
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First draft for adding support for parallel exports based on an int / long splitting column.
Implements #51