You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/07/01 16:56:42 UTC
[GitHub] [pinot] rino-kadijk opened a new pull request, #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
rino-kadijk opened a new pull request, #9008:
URL: https://github.com/apache/pinot/pull/9008
The jdbc client uses AsyncHttpClient without a connect timeout set. This can cause unnecessary latency when connecting to a hostname. In some cases AsyncHttpClient might be able to [retry](https://github.com/AsyncHttpClient/async-http-client/blob/9b7298b8f1cb41fed5fb5a1315267be323c875d6/client/src/main/java/org/asynchttpclient/netty/channel/NettyChannelConnector.java#L104) a second or third address for the host.
This PR:
1. Reduces the timeout when connecting to Pinot fails
2. It enables TLS 1.3 and disables [TLS1.0](https://datatracker.ietf.org/doc/rfc8996/)
3. Adds a user agent header so the server can better understand connection patterns
--
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] KKcorps commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r913395489
##########
pinot-clients/pinot-java-client/src/main/resources/version.properties:
##########
@@ -0,0 +1,20 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+ua=pinot-java/${project.version}
Review Comment:
does `${project.version}` get automatically resolved here or do we need to replace this with correct version on every release
--
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] KKcorps commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r913612591
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -45,6 +46,12 @@ public class PinotControllerTransport {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final long READ_TIMEOUT_MS = 60000L;
+
+ private static final int CONNECT_TIMEOUT_MS = 2000;
+
+ private static final int HANDSHAKE_TIMEOUT_MS = 2000;
Review Comment:
Yep, that works!
--
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] rino-kadijk commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r928685533
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -54,19 +57,19 @@ public class JsonAsyncHttpPinotClientTransport implements PinotClientTransport {
private final Map<String, String> _headers;
private final String _scheme;
- private static final long BROKER_READ_TIMEOUT_MS = 60000L;
- private static final int BROKER_CONNECT_TIMEOUT_MS = 2000;
-
+ private final int _brokerReadTimeout;
private final AsyncHttpClient _httpClient;
public JsonAsyncHttpPinotClientTransport() {
+ _brokerReadTimeout = 60000;
_headers = new HashMap<>();
_scheme = CommonConstants.HTTP_PROTOCOL;
_httpClient = Dsl.asyncHttpClient();
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String scheme,
- @Nullable SSLContext sslContext) {
+ @Nullable SSLContext sslContext, int readTimeout, int connectTimeout, int handshakeTimeout, boolean tlsV10Enabled) {
Review Comment:
I introduced two parameter objects
--
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] KKcorps commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r913395201
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -45,6 +46,12 @@ public class PinotControllerTransport {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final long READ_TIMEOUT_MS = 60000L;
+
+ private static final int CONNECT_TIMEOUT_MS = 2000;
+
+ private static final int HANDSHAKE_TIMEOUT_MS = 2000;
Review Comment:
Can we also add a way to configure these timeouts with jdbc properties. Users may want to change these values.
--
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] rino-kadijk commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r913484843
##########
pinot-clients/pinot-java-client/src/main/resources/version.properties:
##########
@@ -0,0 +1,20 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+ua=pinot-java/${project.version}
Review Comment:
automatically by the resources plugin
--
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] rino-kadijk commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r913486229
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -45,6 +46,12 @@ public class PinotControllerTransport {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final long READ_TIMEOUT_MS = 60000L;
+
+ private static final int CONNECT_TIMEOUT_MS = 2000;
+
+ private static final int HANDSHAKE_TIMEOUT_MS = 2000;
Review Comment:
yes I can add this as jdbc properties, I will keep the defaults low (at 2000), ok?
--
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] apucher commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
apucher commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r925976777
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -54,19 +57,19 @@ public class JsonAsyncHttpPinotClientTransport implements PinotClientTransport {
private final Map<String, String> _headers;
private final String _scheme;
- private static final long BROKER_READ_TIMEOUT_MS = 60000L;
- private static final int BROKER_CONNECT_TIMEOUT_MS = 2000;
-
+ private final int _brokerReadTimeout;
private final AsyncHttpClient _httpClient;
public JsonAsyncHttpPinotClientTransport() {
+ _brokerReadTimeout = 60000;
_headers = new HashMap<>();
_scheme = CommonConstants.HTTP_PROTOCOL;
_httpClient = Dsl.asyncHttpClient();
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String scheme,
- @Nullable SSLContext sslContext) {
+ @Nullable SSLContext sslContext, int readTimeout, int connectTimeout, int handshakeTimeout, boolean tlsV10Enabled) {
Review Comment:
instead of passing a boolean flag for tls 1.0, why not pass a list of protocols?
--
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] KKcorps merged pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps merged PR #9008:
URL: https://github.com/apache/pinot/pull/9008
--
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] KKcorps commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r925337504
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -45,6 +46,12 @@ public class PinotControllerTransport {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final long READ_TIMEOUT_MS = 60000L;
+
+ private static final int CONNECT_TIMEOUT_MS = 2000;
+
+ private static final int HANDSHAKE_TIMEOUT_MS = 2000;
Review Comment:
yes
--
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] rino-kadijk commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r914219956
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -45,6 +46,12 @@ public class PinotControllerTransport {
private static final Logger LOGGER = LoggerFactory.getLogger(PinotControllerTransport.class);
+ private static final long READ_TIMEOUT_MS = 60000L;
+
+ private static final int CONNECT_TIMEOUT_MS = 2000;
+
+ private static final int HANDSHAKE_TIMEOUT_MS = 2000;
Review Comment:
I added configuration properties, should we document them?
--
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 #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9008:
URL: https://github.com/apache/pinot/pull/9008#issuecomment-1172648744
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9008](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7ef7be4) into [master](https://codecov.io/gh/apache/pinot/commit/d4a64e3853102b220cde905f79eaede0054bc43b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d4a64e3) will **decrease** coverage by `0.07%`.
> The diff coverage is `78.94%`.
```diff
@@ Coverage Diff @@
## master #9008 +/- ##
============================================
- Coverage 70.07% 69.99% -0.08%
- Complexity 4733 4956 +223
============================================
Files 1826 1826
Lines 95974 95992 +18
Branches 14350 14350
============================================
- Hits 67249 67193 -56
- Misses 24085 24155 +70
- Partials 4640 4644 +4
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.30% <36.84%> (-0.11%)` | :arrow_down: |
| integration2 | `24.63% <78.94%> (-0.15%)` | :arrow_down: |
| unittests1 | `66.83% <ø> (ø)` | |
| unittests2 | `15.34% <78.94%> (+<0.01%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `49.36% <77.77%> (+2.88%)` | :arrow_up: |
| [...ot/client/controller/PinotControllerTransport.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L2NvbnRyb2xsZXIvUGlub3RDb250cm9sbGVyVHJhbnNwb3J0LmphdmE=) | `45.00% <80.00%> (+7.00%)` | :arrow_up: |
| [...data/manager/realtime/DefaultSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvRGVmYXVsdFNlZ21lbnRDb21taXR0ZXIuamF2YQ==) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [...ller/helix/core/minion/TaskTypeMetricsUpdater.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrVHlwZU1ldHJpY3NVcGRhdGVyLmphdmE=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `43.56% <0.00%> (-18.82%)` | :arrow_down: |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `88.23% <0.00%> (-11.77%)` | :arrow_down: |
| [.../filter/predicate/InPredicateEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL0luUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `68.80% <0.00%> (-9.61%)` | :arrow_down: |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `51.88% <0.00%> (-6.61%)` | :arrow_down: |
| [...ntroller/helix/core/minion/TaskMetricsEmitter.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrTWV0cmljc0VtaXR0ZXIuamF2YQ==) | `86.04% <0.00%> (-4.66%)` | :arrow_down: |
| [...lter/predicate/NotInPredicateEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL05vdEluUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `74.62% <0.00%> (-4.48%)` | :arrow_down: |
| ... and [18 more](https://codecov.io/gh/apache/pinot/pull/9008/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [d4a64e3...7ef7be4](https://codecov.io/gh/apache/pinot/pull/9008?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #9008:
URL: https://github.com/apache/pinot/pull/9008#issuecomment-1190656980
cc @apucher to also take a look
--
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] rino-kadijk commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
rino-kadijk commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r926918621
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -54,19 +57,19 @@ public class JsonAsyncHttpPinotClientTransport implements PinotClientTransport {
private final Map<String, String> _headers;
private final String _scheme;
- private static final long BROKER_READ_TIMEOUT_MS = 60000L;
- private static final int BROKER_CONNECT_TIMEOUT_MS = 2000;
-
+ private final int _brokerReadTimeout;
private final AsyncHttpClient _httpClient;
public JsonAsyncHttpPinotClientTransport() {
+ _brokerReadTimeout = 60000;
_headers = new HashMap<>();
_scheme = CommonConstants.HTTP_PROTOCOL;
_httpClient = Dsl.asyncHttpClient();
}
public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String scheme,
- @Nullable SSLContext sslContext) {
+ @Nullable SSLContext sslContext, int readTimeout, int connectTimeout, int handshakeTimeout, boolean tlsV10Enabled) {
Review Comment:
I could make it a list, but it might be a bit complicated to pass that as a JDBC string argument. It might also introduce possible misconfiguration. A list is more flexible, so I could try to parse it from the connection string. Let me know what you prefer.
--
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] KKcorps commented on a diff in pull request #9008: Add TLS1.3, Remove TLS1.0, Add User Agent Header, Add Timeouts for Pi…
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9008:
URL: https://github.com/apache/pinot/pull/9008#discussion_r932993337
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransportFactory.java:
##########
@@ -60,4 +72,18 @@ public SSLContext getSslContext() {
public void setSslContext(SSLContext sslContext) {
_sslContext = sslContext;
}
+
+ public JsonAsyncHttpPinotClientTransportFactory withConnectionProperties(Properties properties) {
+ _readTimeout = Integer.parseInt(properties.getProperty("brokerReadTimeout",
Review Comment:
Since we expect timeouts to be in milliseconds, we should add this to property name as well. e.g. `brokerReadTimeoutMs`
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java:
##########
@@ -90,14 +95,32 @@ public JsonAsyncHttpPinotClientTransport(Map<String, String> headers, String sch
builder.setSslContext(sslContext);
}
+ builder.setReadTimeout(connectionTimeouts.getReadTimeout())
+ .setConnectTimeout(connectionTimeouts.getConnectTimeout())
+ .setHandshakeTimeout(connectionTimeouts.getHandshakeTimeout())
+ .setUserAgent(getUserAgentVersionFromClassPath())
+ .setEnabledProtocols(tlsProtocols.getEnabledProtocols().toArray(new String[0]));
_httpClient = Dsl.asyncHttpClient(builder.build());
}
+ private String getUserAgentVersionFromClassPath() {
+ Properties userAgentProperties = new Properties();
+ try {
+ userAgentProperties.load(JsonAsyncHttpPinotClientTransport.class.getClassLoader()
+ .getResourceAsStream("version.properties"));
+ } catch (IOException e) {
+ LOGGER.info("Unable to set user agent version");
Review Comment:
nit: `LOGGER.warn`
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/ConnectionTimeouts.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.client;
+
+/**
+ * Connections time out for AsyncHttpClient
+ */
+public class ConnectionTimeouts {
+ private final int _readTimeout;
Review Comment:
nit: we can provide timeunit in the variable for better clarity. e.g. `_readTimeoutMs` if we expect it in milliseconds
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransportFactory.java:
##########
@@ -56,4 +71,18 @@ public SSLContext getSslContext() {
public void setSslContext(SSLContext sslContext) {
_sslContext = sslContext;
}
+
+ public PinotControllerTransportFactory withConnectionProperties(Properties properties) {
+ _readTimeout = Integer.parseInt(properties.getProperty("controllerReadTimeout",
Review Comment:
Same as before, property names should reflect time units if it is not configurable
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -68,9 +62,26 @@ public PinotControllerTransport(Map<String, String> headers, String scheme,
builder.setSslContext(new JdkSslContext(sslContext, true, ClientAuth.OPTIONAL));
}
+ builder.setReadTimeout(connectionTimeouts.getReadTimeout())
+ .setConnectTimeout(connectionTimeouts.getConnectTimeout())
+ .setHandshakeTimeout(connectionTimeouts.getHandshakeTimeout())
+ .setUserAgent(getUserAgentVersionFromClassPath())
+ .setEnabledProtocols(tlsProtocols.getEnabledProtocols().toArray(new String[0]));
+
_httpClient = Dsl.asyncHttpClient(builder.build());
}
+ private String getUserAgentVersionFromClassPath() {
+ Properties userAgentProperties = new Properties();
+ try {
+ userAgentProperties.load(JsonAsyncHttpPinotClientTransport.class.getClassLoader()
+ .getResourceAsStream("version.properties"));
+ } catch (IOException e) {
+ LOGGER.info("Unable to set user agent version");
Review Comment:
should be `LOGGER..warn`
##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/controller/PinotControllerTransport.java:
##########
@@ -68,9 +62,26 @@ public PinotControllerTransport(Map<String, String> headers, String scheme,
builder.setSslContext(new JdkSslContext(sslContext, true, ClientAuth.OPTIONAL));
}
+ builder.setReadTimeout(connectionTimeouts.getReadTimeout())
+ .setConnectTimeout(connectionTimeouts.getConnectTimeout())
+ .setHandshakeTimeout(connectionTimeouts.getHandshakeTimeout())
+ .setUserAgent(getUserAgentVersionFromClassPath())
+ .setEnabledProtocols(tlsProtocols.getEnabledProtocols().toArray(new String[0]));
+
_httpClient = Dsl.asyncHttpClient(builder.build());
}
+ private String getUserAgentVersionFromClassPath() {
+ Properties userAgentProperties = new Properties();
+ try {
+ userAgentProperties.load(JsonAsyncHttpPinotClientTransport.class.getClassLoader()
Review Comment:
No need to import `JsonAsyncHttpPinotClientTransport` for this
--
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