You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by "chengyouling (via GitHub)" <gi...@apache.org> on 2023/02/06 13:19:42 UTC
[GitHub] [servicecomb-java-chassis] chengyouling opened a new pull request, #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
chengyouling opened a new pull request, #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623
Signed-off-by: Youling Cheng<sp...@163.com>
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] chengyouling commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "chengyouling (via GitHub)" <gi...@apache.org>.
chengyouling commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1098128143
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/RestTransportClient.java:
##########
@@ -77,7 +77,7 @@ private static HttpClientOptions createHttpClientOptions() {
HttpClientOptions httpClientOptions = new HttpClientOptions();
httpClientOptions.setMaxPoolSize(TransportClientConfig.getConnectionMaxPoolSize())
.setIdleTimeout(TransportClientConfig.getConnectionIdleTimeoutInSeconds())
- .setKeepAliveTimeout(TransportClientConfig.getConnectionIdleTimeoutInSeconds())
+ .setKeepAliveTimeout(TransportClientConfig.getHttp1ConnectionKeepAliveTimeoutInSeconds())
Review Comment:
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] liubao68 merged pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "liubao68 (via GitHub)" <gi...@apache.org>.
liubao68 merged PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] chengyouling commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "chengyouling (via GitHub)" <gi...@apache.org>.
chengyouling commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1098226206
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/TransportClientConfig.java:
##########
@@ -86,6 +86,34 @@ public static boolean getConnectionKeepAlive() {
.get();
}
+ public static int getHttp1ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http1.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
+ if (result > 1) {
+ return 0;
Review Comment:
fixed
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] liubao68 commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "liubao68 (via GitHub)" <gi...@apache.org>.
liubao68 commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1102210489
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/TransportClientConfig.java:
##########
@@ -86,6 +86,34 @@ public static boolean getConnectionKeepAlive() {
.get();
}
+ public static int getHttp1ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http1.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
+ if (result > 1) {
+ return result - 1;
+ }
+ return result;
+ }
+
+ public static int getHttp2ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http2.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
Review Comment:
should be getHttp2ConnectionIdleTimeoutInSeconds?
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] codecov-commenter commented on pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#issuecomment-1427468395
# [Codecov](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?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 [#3623](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6701b96) into [1.3.x](https://codecov.io/gh/apache/servicecomb-java-chassis/commit/585dfb46bccdcafa19dd68c34ff96740efafe270?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (585dfb4) will **decrease** coverage by `0.01%`.
> The diff coverage is `60.00%`.
> :exclamation: Current head 6701b96 differs from pull request most recent head 9181d63. Consider uploading reports for the commit 9181d63 to get more accurate results
```diff
@@ Coverage Diff @@
## 1.3.x #3623 +/- ##
============================================
- Coverage 82.96% 82.96% -0.01%
Complexity 746 746
============================================
Files 1232 1232
Lines 33041 33059 +18
Branches 2993 2997 +4
============================================
+ Hits 27414 27426 +12
- Misses 4445 4449 +4
- Partials 1182 1184 +2
```
| [Impacted Files](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...b/transport/rest/client/TransportClientConfig.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhbnNwb3J0cy90cmFuc3BvcnQtcmVzdC90cmFuc3BvcnQtcmVzdC1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NlcnZpY2Vjb21iL3RyYW5zcG9ydC9yZXN0L2NsaWVudC9UcmFuc3BvcnRDbGllbnRDb25maWcuamF2YQ==) | `81.13% <55.55%> (-13.16%)` | :arrow_down: |
| [...omb/transport/rest/client/RestTransportClient.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhbnNwb3J0cy90cmFuc3BvcnQtcmVzdC90cmFuc3BvcnQtcmVzdC1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NlcnZpY2Vjb21iL3RyYW5zcG9ydC9yZXN0L2NsaWVudC9SZXN0VHJhbnNwb3J0Q2xpZW50LmphdmE=) | `98.00% <100.00%> (ø)` | |
| [...egistry/client/LocalServiceRegistryClientImpl.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmljZS1yZWdpc3RyeS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VydmljZWNvbWIvc2VydmljZXJlZ2lzdHJ5L2NsaWVudC9Mb2NhbFNlcnZpY2VSZWdpc3RyeUNsaWVudEltcGwuamF2YQ==) | `80.09% <0.00%> (+0.46%)` | :arrow_up: |
| [.../org/apache/servicecomb/provider/pojo/Invoker.java](https://codecov.io/gh/apache/servicecomb-java-chassis/pull/3623?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHJvdmlkZXJzL3Byb3ZpZGVyLXBvam8vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NlcnZpY2Vjb21iL3Byb3ZpZGVyL3Bvam8vSW52b2tlci5qYXZh) | `95.77% <0.00%> (+1.40%)` | :arrow_up: |
: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=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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] chengyouling commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "chengyouling (via GitHub)" <gi...@apache.org>.
chengyouling commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1098128040
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/TransportClientConfig.java:
##########
@@ -86,6 +86,34 @@ public static boolean getConnectionKeepAlive() {
.get();
}
+ public static int getHttp1ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http1.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
+ if (result > 1) {
+ return 0;
Review Comment:
because HttpClientOptions will check value mast above 0 when setting value.
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] liubao68 commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "liubao68 (via GitHub)" <gi...@apache.org>.
liubao68 commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1097394963
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/RestTransportClient.java:
##########
@@ -77,7 +77,7 @@ private static HttpClientOptions createHttpClientOptions() {
HttpClientOptions httpClientOptions = new HttpClientOptions();
httpClientOptions.setMaxPoolSize(TransportClientConfig.getConnectionMaxPoolSize())
.setIdleTimeout(TransportClientConfig.getConnectionIdleTimeoutInSeconds())
- .setKeepAliveTimeout(TransportClientConfig.getConnectionIdleTimeoutInSeconds())
+ .setKeepAliveTimeout(TransportClientConfig.getHttp1ConnectionKeepAliveTimeoutInSeconds())
Review Comment:
This change should create PR to master/2.8.x/1.3.x
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] liubao68 commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "liubao68 (via GitHub)" <gi...@apache.org>.
liubao68 commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1097396374
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/TransportClientConfig.java:
##########
@@ -86,6 +86,34 @@ public static boolean getConnectionKeepAlive() {
.get();
}
+ public static int getHttp1ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http1.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
+ if (result > 1) {
+ return 0;
Review Comment:
why return 0?
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [servicecomb-java-chassis] chengyouling commented on a diff in pull request #3623: [#3607] fix idle time out and keep alive timeout not properly set pro…
Posted by "chengyouling (via GitHub)" <gi...@apache.org>.
chengyouling commented on code in PR #3623:
URL: https://github.com/apache/servicecomb-java-chassis/pull/3623#discussion_r1098128040
##########
transports/transport-rest/transport-rest-client/src/main/java/org/apache/servicecomb/transport/rest/client/TransportClientConfig.java:
##########
@@ -86,6 +86,34 @@ public static boolean getConnectionKeepAlive() {
.get();
}
+ public static int getHttp1ConnectionKeepAliveTimeoutInSeconds() {
+ int result = DynamicPropertyFactory.getInstance()
+ .getIntProperty("servicecomb.rest.client.http1.connection.keepAliveTimeoutInSeconds", -1)
+ .get();
+ if (result >= 0) {
+ return result;
+ }
+ result = getConnectionIdleTimeoutInSeconds();
+ if (result > 1) {
+ return 0;
Review Comment:
because HttpClientOptions will check value mast above 0 when setting value.
--
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@servicecomb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org