-
Notifications
You must be signed in to change notification settings - Fork 513
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
Allow users to configure fetch and batch size for jdbc reads and writes #1314
Conversation
Codecov Report
@@ Coverage Diff @@
## nio #1314 +/- ##
==========================================
- Coverage 81.73% 81.67% -0.07%
==========================================
Files 146 146
Lines 4244 4250 +6
Branches 373 313 -60
==========================================
+ Hits 3469 3471 +2
- Misses 775 779 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you PR against nio
branch instead since we're gonna promote that into master
soon.
a14a6e7
to
da8aa14
Compare
@nevillelyh Done! |
@@ -65,7 +65,8 @@ package object jdbc { | |||
case class JdbcReadOptions[T](connectionOptions: JdbcConnectionOptions, | |||
query: String, | |||
statementPreparator: PreparedStatement => Unit = null, | |||
rowMapper: ResultSet => T) extends JdbcIoOptions | |||
rowMapper: ResultSet => T, | |||
fetchSize: Option[Int] = None) extends JdbcIoOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of requiring the user to wrap Int
in Some
here, what about use Int
directly with a default of -1
, which skips setting the fetch size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, yes agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except the minor API design choice.
dd331f4
to
b444ff9
Compare
package object jdbc { | ||
|
||
type JdbcIO[T] = jdbc.nio.JdbcIO[T] | ||
val JdbcIO = jdbc.nio.JdbcIO | ||
|
||
private[jdbc] val USE_BEAM_DEFAULT_BATCH_SIZE = -1L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't go crazy as de-magic-fy these since -1
is a pretty common pattern but that's just me ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought it is more informative what actually -1 means here.
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
…es (#1314) * Allow users to configure fetch and batch size for jdbc read and write * clean the api a bit
Fixes #1311