You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2024/10/09 02:15:03 UTC
[PR] Added http client connection reusable strategy [pinot]
abhioncbr opened a new pull request, #14191:
URL: https://github.com/apache/pinot/pull/14191
TODO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2452698588
We will wait for http5-client 5.4.2 version, to check if the fix is there for Http1.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2436124636
This is the highlighted release note, not sure if related to the issue we encountered:
```
IMPORTANT! The new cache entry serialization format is incompatible with earlier
versions of HttpClient Cache. Persistent caches (file system based, Memcached, EhCAche
with object serialization) created with any earlier version MUST be flushed and re-populated
or the cache backend MUST be configured to use the old, deprecated cache entry serializer.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-4112943433
Now we are on version `5.6`. @abhioncbr Can you rebase this change and see if it works now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14191:
URL: https://github.com/apache/pinot/pull/14191#discussion_r1815521655
##########
pinot-common/src/main/java/org/apache/pinot/common/http/MultiHttpRequest.java:
##########
@@ -116,7 +117,8 @@ public <T extends HttpUriRequestBase> CompletionService<MultiHttpRequestResponse
HttpClients.custom().setConnectionManager(_connectionManager).setDefaultRequestConfig(defaultRequestConfig);
CompletionService<MultiHttpRequestResponse> completionService = new ExecutorCompletionService<>(_executor);
- CloseableHttpClient client = httpClientBuilder.build();
+ CloseableHttpClient client = httpClientBuilder
+ .setConnectionReuseStrategy((request, response, context) -> false).build();
Review Comment:
Does it work if you revert this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2436418018
> This is the highlighted release note, not sure if related to the issue we encountered:
>
> ```
> IMPORTANT! The new cache entry serialization format is incompatible with earlier
> versions of HttpClient Cache. Persistent caches (file system based, Memcached, EhCAche
> with object serialization) created with any earlier version MUST be flushed and re-populated
> or the cache backend MUST be configured to use the old, deprecated cache entry serializer.
> ```
Yes, I am following the release notes and saw this. They made the cache change in release 5.4.2-alpha, and we are seeing these issues from that version. In my understanding, we aren't using HTTP caching, and unit test cases that are not cache-based are failing. I am unable to pinpoint the exact issue.
It seems like the HTTP 2 protocol version looks good as per our test suites, and it's more performant than HTTP 1.1, maybe we can give this solution a try?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2435352441
> Thanks for debugging this!
>
> Can you elaborate more on the root cause? Since the bug was fixed in the new version, why would our test fail? Is there extra overhead if we do not reuse connection?
IMO, connection creation is a time-consuming process, and hence, not reusing it would degrade the performance. I have reverted the changes. I am not sure if switching to HTTP2 is an acceptable solution or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added http client connection reusable strategy [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2401168248
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14191?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 55.33%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ead2c41`)](https://app.codecov.io/gh/apache/pinot/commit/ead2c410c8ef418ba9aa7739dc2bfb5cbcd61fed?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1161 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (ead2c41). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (ead2c41) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14191 +/- ##
============================================
- Coverage 61.75% 55.33% -6.42%
- Complexity 207 704 +497
============================================
Files 2436 2067 -369
Lines 133233 109157 -24076
Branches 20636 17284 -3352
============================================
- Hits 82274 60402 -21872
+ Misses 44911 43928 -983
+ Partials 6048 4827 -1221
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.33% <100.00%> (-6.30%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.33% <100.00%> (-6.42%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.33% <100.00%> (-6.42%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.33% <100.00%> (-6.42%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.33% <100.00%> (+8.44%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14191/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14191?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Added http client connection reusable strategy [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2406043509
Thanks for debugging this!
Can you elaborate more on the root cause? Since the bug was fixed in the new version, why would our test fail?
Is there extra overhead if we do not reuse connection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Upgrade http5 Client to 5.4 and using Http 2 Protocol [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14191:
URL: https://github.com/apache/pinot/pull/14191#issuecomment-2436122218
For reference: [5.4 release note](https://downloads.apache.org/httpcomponents/httpclient/RELEASE_NOTES-5.4.x.txt)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org