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

[pinot-spark-connector] Bump spark connector max inbound message size #9475

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

cbalci
Copy link
Contributor

@cbalci cbalci commented Sep 27, 2022

Updating the GRPC client configuration used by the Pinot Spark Connector to use Int.MaxValue for message size which effectively bumps the max message size from 4MB to ~2GBs.
Spark-Connector is intended for potentially large reads, so 4MB can be a bottleneck for certain use cases (very wide tables).

Testing:
I've tested this locally with chunk sizes exceeding 4MB. I'm not adding an explicit test case not to unnecessarily burden the test runner.

bugfix performance

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Merging #9475 (89982f5) into master (d4392f9) will increase coverage by 43.95%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #9475       +/-   ##
=============================================
+ Coverage     25.90%   69.85%   +43.95%     
- Complexity       44     4750     +4706     
=============================================
  Files          1890     1911       +21     
  Lines        101219   101895      +676     
  Branches      15383    15456       +73     
=============================================
+ Hits          26220    71181    +44961     
+ Misses        72344    25688    -46656     
- Partials       2655     5026     +2371     
Flag Coverage Δ
integration1 25.97% <ø> (+0.07%) ⬆️
integration2 24.74% <ø> (?)
unittests1 67.10% <ø> (?)
unittests2 15.50% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...inion/event/DefaultMinionEventObserverFactory.java 50.00% <0.00%> (-16.67%) ⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java 58.49% <0.00%> (-1.89%) ⬇️
.../pinot/minion/taskfactory/TaskFactoryRegistry.java 79.41% <0.00%> (-0.30%) ⬇️
...nandpush/SegmentGenerationAndPushTaskExecutor.java 66.90% <0.00%> (-0.24%) ⬇️
...roller/api/resources/PinotTaskRestletResource.java 2.98% <0.00%> (-0.02%) ⬇️
...pache/pinot/core/query/utils/idset/EmptyIdSet.java 25.00% <0.00%> (ø)
...inion/api/resources/PinotTaskProgressResource.java 0.00% <0.00%> (ø)
...anager/realtime/SegmentBuildTimeLeaseExtender.java 63.23% <0.00%> (ø)
...ing/instanceselector/BalancedInstanceSelector.java 84.61% <0.00%> (ø)
...ent/local/data/manager/TableDataManagerConfig.java 0.00% <0.00%> (ø)
... and 1400 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@Jackie-Jiang Jackie-Jiang merged commit 2daa863 into apache:master Sep 28, 2022
61yao pushed a commit to 61yao/pinot that referenced this pull request Oct 3, 2022
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