Skip to content

allow to customize the httpclient used to create S3PinotFS#10369

Merged
Jackie-Jiang merged 5 commits intoapache:masterfrom
klsince:customize_httpclient_s3pinotfs
Mar 7, 2023
Merged

allow to customize the httpclient used to create S3PinotFS#10369
Jackie-Jiang merged 5 commits intoapache:masterfrom
klsince:customize_httpclient_s3pinotfs

Conversation

@klsince
Copy link
Contributor

@klsince klsince commented Mar 2, 2023

This PR exposes some configs to customize the httpclient used to create s3client, when to init the S3PinotFS.

Release Note

the exposed httpclient configs are
httpclient.maxConnections, e.g. 100 (this is more required for now. when we increased helixExecutorTask thread number via STATE_TRANSITION.maxThreads to load segments with more threads, some threads threw exceptions about timeout to get a http conn to download raw segments from deep store)

httpclient.socketTimeout, e.g. 10s
httpclient.connectionTimeout, e.g. 1m20s
httpclient.connectionTimeToLive, e.g. ..
httpclient.connectionAcquisitionTimeout, e.g. ..

Copy link
Contributor

@npawar npawar left a comment

Choose a reason for hiding this comment

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

lgtm.
I didn't realize we've already introduced PT notations in DateTimeFunctions. Was going to suggest we stick to the period strings we use in other places like realtime thresholds, periodic task frequencies (12h, 3m30s). But this sounds fine to me.
Please add these new props to https://docs.pinot.apache.org/basics/data-import/pinot-file-system/amazon-s3#server-config

@klsince klsince force-pushed the customize_httpclient_s3pinotfs branch from 6272a68 to d921443 Compare March 2, 2023 18:03
@klsince
Copy link
Contributor Author

klsince commented Mar 2, 2023

I didn't realize we've already introduced PT notations in DateTimeFunctions. ...

Thanks for the context. I was using PT format as Duration is what the httpClientBuilder takes. I'll make it compatible with the two formats, as it's better to keep format of timeout/period configs consistent

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.44%. Comparing base (5d0089a) to head (e659bc0).
Report is 2817 commits behind head on master.

Files with missing lines Patch % Lines
...a/org/apache/pinot/plugin/filesystem/S3Config.java 86.84% 0 Missing and 5 partials ⚠️
.../org/apache/pinot/plugin/filesystem/S3PinotFS.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #10369      +/-   ##
============================================
+ Coverage     67.87%   70.44%   +2.57%     
- Complexity     5742     6005     +263     
============================================
  Files          1521     2036     +515     
  Lines         80305   110444   +30139     
  Branches      12826    16793    +3967     
============================================
+ Hits          54506    77804   +23298     
- Misses        21957    27202    +5245     
- Partials       3842     5438    +1596     
Flag Coverage Δ
integration1 24.45% <0.00%> (?)
integration2 24.45% <0.00%> (?)
unittests1 67.88% <ø> (+0.01%) ⬆️
unittests2 13.78% <82.50%> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@klsince klsince force-pushed the customize_httpclient_s3pinotfs branch 2 times, most recently from 4ce393a to 9b002b2 Compare March 2, 2023 23:32
Copy link
Contributor

@npawar npawar left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

@klsince klsince force-pushed the customize_httpclient_s3pinotfs branch 3 times, most recently from 200fddc to 79270fa Compare March 6, 2023 22:55
@klsince klsince force-pushed the customize_httpclient_s3pinotfs branch from 79270fa to fa41785 Compare March 7, 2023 05:59
@Jackie-Jiang Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Mar 7, 2023
@Jackie-Jiang Jackie-Jiang merged commit 585f62a into apache:master Mar 7, 2023
@klsince klsince deleted the customize_httpclient_s3pinotfs branch March 8, 2023 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants