You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/29 19:56:05 UTC
[GitHub] [druid] gianm opened a new pull request #10098: Update Jetty to 9.4.30.v20200611.
gianm opened a new pull request #10098:
URL: https://github.com/apache/druid/pull/10098
This is the latest version currently available in the 9.4.x line.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10098: Update Jetty to 9.4.30.v20200611.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10098:
URL: https://github.com/apache/druid/pull/10098#issuecomment-653109512
It looks like CI passed except for code coverage. Here is the report. It's all guice stuff that should be tested by the integration tests. Do people mind if I add suppressions for these?
```
Diff coverage statistics:
------------------------------------------------------------------------------
| lines | branches | functions | path
------------------------------------------------------------------------------
| 100% (2/2) | 100% (0/0) | 100% (1/1) | org/apache/druid/server/initialization/jetty/JettyServerModule.java
| 0% (0/1) | 100% (0/0) | 100% (0/0) | org/apache/druid/server/initialization/jetty/CliIndexerServerModule.java
| 0% (0/1) | 100% (0/0) | 100% (1/1) | org/apache/druid/server/initialization/jetty/ChatHandlerServerModule.java
| 0% (0/11) | 0% (0/2) | 60% (3/5) | org/apache/druid/guice/http/JettyHttpClientModule.java
| 72% (8/11) | 0% (0/2) | 33% (2/6) | org/apache/druid/guice/http/HttpClientModule.java
| 100% (1/1) | 100% (0/0) | 50% (1/2) | org/apache/druid/guice/http/AbstractHttpClientProvider.java
------------------------------------------------------------------------------
Total diff coverage:
- lines: 40% (11/27)
- branches: 0% (0/4)
- functions: 53% (8/15)
ERROR: Insufficient line coverage of 40% (11/27). Required 50%.
ERROR: Insufficient branch coverage of 0% (0/4). Required 50%.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10098: Update Jetty to 9.4.30.v20200611.
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10098:
URL: https://github.com/apache/druid/pull/10098#issuecomment-654385127
> It looks like CI passed except for code coverage. Here is the report. It's all guice stuff that should be tested by the integration tests. Do people mind if I add suppressions for these?
>
> ```
> Diff coverage statistics:
>
> ------------------------------------------------------------------------------
> | lines | branches | functions | path
> ------------------------------------------------------------------------------
> | 100% (2/2) | 100% (0/0) | 100% (1/1) | org/apache/druid/server/initialization/jetty/JettyServerModule.java
> | 0% (0/1) | 100% (0/0) | 100% (0/0) | org/apache/druid/server/initialization/jetty/CliIndexerServerModule.java
> | 0% (0/1) | 100% (0/0) | 100% (1/1) | org/apache/druid/server/initialization/jetty/ChatHandlerServerModule.java
> | 0% (0/11) | 0% (0/2) | 60% (3/5) | org/apache/druid/guice/http/JettyHttpClientModule.java
> | 72% (8/11) | 0% (0/2) | 33% (2/6) | org/apache/druid/guice/http/HttpClientModule.java
> | 100% (1/1) | 100% (0/0) | 50% (1/2) | org/apache/druid/guice/http/AbstractHttpClientProvider.java
> ------------------------------------------------------------------------------
>
> Total diff coverage:
> - lines: 40% (11/27)
> - branches: 0% (0/4)
> - functions: 53% (8/15)
>
> ERROR: Insufficient line coverage of 40% (11/27). Required 50%.
> ERROR: Insufficient branch coverage of 0% (0/4). Required 50%.
> ```
These suppressions seem reasonable to me. The only thing I'm slightly concerned about is there is some logic in the Modules you listed that bind different things based on some input. I think that level of detailed testing would be too hard in the integration tests, but a lot easier to enforce in unit tests. For example, in CliIndexerServerModule:
```
// Use an equal number of threads for chat handler and non-chat handler requests.
int serverHttpNumThreads;
if (properties.getProperty(SERVER_HTTP_NUM_THREADS_PROPERTY) == null) {
serverHttpNumThreads = ServerConfig.getDefaultNumThreads();
} else {
serverHttpNumThreads = Integer.parseInt(properties.getProperty(SERVER_HTTP_NUM_THREADS_PROPERTY));
}
```
Validating this behavior would be a lot easier in unit tests than integration tests. However, there are other parts of the module that aren't very unit testable, like validating a QoS filter was bound correctly
```
JettyBindings.addQosFilter(
binder,
"/druid/worker/v1/chat/*",
serverHttpNumThreads
);
```
Given this, I think it's a reasonable tradeoff to add suppressions for these classes since the module code itself is simple to follow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10098: Update Jetty to 9.4.30.v20200611.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10098:
URL: https://github.com/apache/druid/pull/10098#issuecomment-654535632
I agree that a lot of the stuff in those modules would be tough to test directly. It would make more sense, for example, to test the QoS stuff using a load test. I'll add the suppressions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #10098: Update Jetty to 9.4.30.v20200611.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #10098:
URL: https://github.com/apache/druid/pull/10098
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org