You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ankitsultana (via GitHub)" <gi...@apache.org> on 2023/06/12 08:20:03 UTC
[GitHub] [pinot] ankitsultana opened a new pull request, #10895: Add Support for Disabling Default User Agent for Http Client
ankitsultana opened a new pull request, #10895:
URL: https://github.com/apache/pinot/pull/10895
Potential fix for #10894.
The default value for this is `false` and we don't change the default behavior.
**Test Plan**: Will test on local to ensure configs are getting updated.
--
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
[GitHub] [pinot] ankitsultana commented on pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #10895:
URL: https://github.com/apache/pinot/pull/10895#issuecomment-1589897506
@Jackie-Jiang : can you take a look at this again?
I'll run a test on local to verify the configs are indeed working and will get back soon. But lmk if you have any other questions. We are hoping to merge this today.
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10895:
URL: https://github.com/apache/pinot/pull/10895#issuecomment-1586901564
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10895?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10895](https://app.codecov.io/gh/apache/pinot/pull/10895?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d7b2a18) into [master](https://app.codecov.io/gh/apache/pinot/commit/b76f24cb30c7c92c2839faac5ea6a97ac01f2044?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b76f24c) will **decrease** coverage by `34.36%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10895 +/- ##
=============================================
- Coverage 34.47% 0.11% -34.36%
=============================================
Files 2159 2132 -27
Lines 116007 114999 -1008
Branches 17558 17461 -97
=============================================
- Hits 39989 137 -39852
- Misses 72545 114842 +42297
+ Partials 3473 20 -3453
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests2 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (?)` | |
| unittests2temurin17 | `0.11% <0.00%> (?)` | |
| unittests2temurin20 | `0.11% <0.00%> (?)` | |
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.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10895?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ache/pinot/common/utils/http/HttpClientConfig.java](https://app.codecov.io/gh/apache/pinot/pull/10895?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaHR0cC9IdHRwQ2xpZW50Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-68.00%)` | :arrow_down: |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://app.codecov.io/gh/apache/pinot/pull/10895?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-58.50%)` | :arrow_down: |
... and [1128 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10895/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10895:
URL: https://github.com/apache/pinot/pull/10895#discussion_r1227234145
##########
pinot-core/src/main/java/org/apache/pinot/server/realtime/ServerSegmentCompletionProtocolHandler.java:
##########
@@ -52,6 +53,7 @@ public class ServerSegmentCompletionProtocolHandler {
private static final String HTTP_PROTOCOL = CommonConstants.HTTP_PROTOCOL;
private static SSLContext _sslContext;
+ private static HttpClientConfig _httpClientConfig = HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG;
Review Comment:
So right now most of the member vars are stored in a static context in this class, and Pinot server calls `ServerSegmentCompletionProtocolHandler#init` on start-up to make sure everything is assigned some values.
Without setting this to the default value, some of the UTs were failing because we were not initializing `ServerSegmentCompletionProtocolHandler` in them. So I had two options:
1. Either initialize it everywhere in the test
2. or go with a default value.
I picked option-2. lmk if you see any concerns.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10895:
URL: https://github.com/apache/pinot/pull/10895#discussion_r1227219879
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClientConfig.java:
##########
@@ -61,6 +68,10 @@ public static Builder newBuilder(PinotConfiguration pinotConfiguration) {
if (StringUtils.isNotEmpty(maxConnsPerRoute)) {
builder.withMaxConnsPerRoute(Integer.parseInt(maxConnsPerRoute));
}
+ boolean disableDefaultUserAgent = pinotConfiguration.getProperty(DISABLE_DEFAULT_USER_AGENT_CONFIG_NAME, false);
+ if (disableDefaultUserAgent) {
+ builder.withDisableDefaultUserAgent(true);
+ }
Review Comment:
```suggestion
builder.withDisableDefaultUserAgent(disableDefaultUserAgent);
```
##########
pinot-core/src/main/java/org/apache/pinot/server/realtime/ServerSegmentCompletionProtocolHandler.java:
##########
@@ -52,6 +53,7 @@ public class ServerSegmentCompletionProtocolHandler {
private static final String HTTP_PROTOCOL = CommonConstants.HTTP_PROTOCOL;
private static SSLContext _sslContext;
+ private static HttpClientConfig _httpClientConfig = HttpClientConfig.DEFAULT_HTTP_CLIENT_CONFIG;
Review Comment:
I don't follow the changes in this file
--
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
[GitHub] [pinot] chenboat merged pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat merged PR #10895:
URL: https://github.com/apache/pinot/pull/10895
--
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
[GitHub] [pinot] ankitsultana closed pull request #10895: Add Support for Disabling Default User Agent for Http Client
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana closed pull request #10895: Add Support for Disabling Default User Agent for Http Client
URL: https://github.com/apache/pinot/pull/10895
--
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