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

Allowing Jdbc FetchSize Configuration for handling large rows #2028

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

VardhanThigle
Copy link
Contributor

@VardhanThigle VardhanThigle commented Nov 21, 2024

Allowing Jdbc FetchSize Configuration for handling large rows.

Overview

Default fetch size used by JdbcIO is 50_000 rows.
Large (in terms of memory size) rows can lead to memory errors and JdbcIO recommends tuning fetch size in case of memory errors. Please see here.
Here we allow the user to tune the fetch size via parameters.
Auto inference of fetchsize will be taken as a separate task as it needs careful scale testing.

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.97%. Comparing base (40e8298) to head (11bc6c2).
Report is 6 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2028      +/-   ##
============================================
+ Coverage     45.40%   52.97%   +7.56%     
+ Complexity     3680     1371    -2309     
============================================
  Files           843      378     -465     
  Lines         49989    20680   -29309     
  Branches       5261     2092    -3169     
============================================
- Hits          22700    10955   -11745     
+ Misses        25620     9045   -16575     
+ Partials       1669      680     -989     
Components Coverage Δ
spanner-templates 67.99% <100.00%> (+1.26%) ⬆️
spanner-import-export ∅ <ø> (∅)
spanner-live-forward-migration 75.88% <ø> (ø)
spanner-live-reverse-replication 76.65% <ø> (ø)
spanner-bulk-migration 86.41% <100.00%> (+0.04%) ⬆️
Files with missing lines Coverage Δ
...ud/teleport/v2/options/OptionsToConfigBuilder.java 94.62% <100.00%> (+0.11%) ⬆️
...e/reader/auth/dbauth/LocalCredentialsProvider.java 100.00% <100.00%> (ø)
...source/reader/io/jdbc/iowrapper/JdbcIoWrapper.java 93.92% <100.00%> (+0.17%) ⬆️
...splitter/transforms/ReadWithUniformPartitions.java 98.44% <100.00%> (+0.08%) ⬆️
...loud/teleport/v2/templates/PipelineController.java 33.91% <100.00%> (+0.57%) ⬆️

... and 482 files with indirect coverage changes

@VardhanThigle VardhanThigle changed the title [Draft: Not For review] Allowing Jdbc FetchSize Configuration for handling large rows Allowing Jdbc FetchSize Configuration for handling large rows Nov 22, 2024
@VardhanThigle VardhanThigle marked this pull request as ready for review November 22, 2024 04:57
@VardhanThigle VardhanThigle requested a review from a team as a code owner November 22, 2024 04:57
Copy link
Contributor

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

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

Comments around the help text, rest looks fine.

@VardhanThigle VardhanThigle force-pushed the fetchsize branch 2 times, most recently from fcff0c4 to 510bda1 Compare November 22, 2024 09:03
Copy link
Contributor

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

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

LGTM

manitgupta
manitgupta previously approved these changes Nov 25, 2024
Copy link
Contributor

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

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

minor suggestion on config. Rest looks fine.

@VardhanThigle VardhanThigle merged commit c5a74fc into GoogleCloudPlatform:main Nov 28, 2024
14 checks passed
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