You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/19 11:21:00 UTC
[GitHub] [drill] ihuzenko opened a new pull request #2031: DRILL-7650: Add
option to enable Jetty's dump for troubleshooting
ihuzenko opened a new pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031
# [DRILL-7650](https://issues.apache.org/jira/browse/DRILL-7650): Add option to enable Jetty's dump for troubleshooting
## Description
Jetty server implements a useful tool for dumping the current state of the server, but in Drill, it's not possible to use it without code changes. This ticket aims to add option drill.exec.http.jetty.server.dumpAfterStart in Apache Drill.
Another change is to remove the redundant setProtocol(protocol) method call on sslContextFactory instance.
## Documentation
Set drill.exec.http.jetty.server.dumpAfterStart to true if you want to check server state in logs.
## Testing
Checked manually.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on issue #2031: DRILL-7650: Add option
to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on issue #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#issuecomment-601172921
> @ihuzenko, could you please explain, why `sslFactory.setProtocol(sslConf.getProtocol())` the call is excessive? I have looked into the `SslContextFactory` class, and the corresponding field is used in some places, for example in the `load()` method, and its non-default value is set only using its setter.
According to [the doc ](https://www.eclipse.org/jetty/documentation/9.4.x/configuring-ssl.html#configuring-sslcontextfactory) it's enough to just set included 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on issue #2031: DRILL-7650: Add option
to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on issue #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#issuecomment-601196180
@vvysotskyi , prior to [DRILL-7625](https://issues.apache.org/jira/browse/DRILL-7625) the line didn't exist so I hope it won't hurt anybody.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2031:
DRILL-7650: Add option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#discussion_r395024249
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
##########
@@ -163,6 +163,7 @@ public void start() throws Exception {
embeddedJetty.addConnector(connector);
final boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+ embeddedJetty.setDumpAfterStart(config.getBoolean(ExecConstants.HTTP_JETTY_SERVER_DUMP_AFTER_START));
Review comment:
done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko edited a comment on issue #2031: DRILL-7650: Add
option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
ihuzenko edited a comment on issue #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#issuecomment-601196180
@vvysotskyi , prior to [DRILL-7625](https://issues.apache.org/jira/browse/DRILL-7625) the line ```sslFactory.setProtocol(sslConf.getProtocol());``` didn't exist so I hope it won't hurt anybody.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2031:
DRILL-7650: Add option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#discussion_r395001902
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
##########
@@ -163,6 +163,7 @@ public void start() throws Exception {
embeddedJetty.addConnector(connector);
final boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+ embeddedJetty.setDumpAfterStart(config.getBoolean(ExecConstants.HTTP_JETTY_SERVER_DUMP_AFTER_START));
Review comment:
Could you please move it either above port hunting logic or below it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2031:
DRILL-7650: Add option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#discussion_r395021565
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
##########
@@ -163,6 +163,7 @@ public void start() throws Exception {
embeddedJetty.addConnector(connector);
final boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+ embeddedJetty.setDumpAfterStart(config.getBoolean(ExecConstants.HTTP_JETTY_SERVER_DUMP_AFTER_START));
Review comment:
It is a minor comment. Placing it after declaring the `portHunt` variable and before the loop where port hunting happens enforces to think that it should be somehow connected with port hunting. Please move it above declaring the `portHunt` variable if possible.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva merged pull request #2031: DRILL-7650: Add
option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva merged pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #2031:
DRILL-7650: Add option to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#discussion_r395015959
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java
##########
@@ -163,6 +163,7 @@ public void start() throws Exception {
embeddedJetty.addConnector(connector);
final boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+ embeddedJetty.setDumpAfterStart(config.getBoolean(ExecConstants.HTTP_JETTY_SERVER_DUMP_AFTER_START));
Review comment:
Could you please explain why? The method called already before the port hunting loop. Moving it below doesn't make sense since it will be too late to set the flag on ```embeddedJetty``` object.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on issue #2031: DRILL-7650: Add option
to enable Jetty's dump for troubleshooting
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on issue #2031: DRILL-7650: Add option to enable Jetty's dump for troubleshooting
URL: https://github.com/apache/drill/pull/2031#issuecomment-601187835
@ihuzenko, thanks for the reference. This doc is for configuring TLS protocols. The default value of `_sslProtocol` is `TLS`, so perhaps that's why it wasn't documented there. I'm not an expert in SLL/TLS, but I'm afraid this change may break backward compatibility for users with specific SSL configs.
----------------------------------------------------------------
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
With regards,
Apache Git Services