[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1148


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171989096
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -424,10 +418,11 @@ private ServerConnector createHttpsConnector(int 
port) throws Exception {
* @return Initialized {@link ServerConnector} instance for HTTP 
connections.
* @throws Exception
*/
-  private ServerConnector createHttpConnector(int port) throws Exception {
+  private ServerConnector createHttpConnector(int port, int acceptors, int 
selectors) throws Exception {
 logger.info("Setting up HTTP connector for web server");
 final HttpConfiguration httpConfig = new HttpConfiguration();
-final ServerConnector httpConnector = new 
ServerConnector(embeddedJetty, new HttpConnectionFactory(httpConfig));
+final ServerConnector httpConnector =
+new ServerConnector(embeddedJetty, null, null, null, acceptors, 
selectors, new HttpConnectionFactory(httpConfig));
--- End diff --

I don't think there is such constructor (only one that takes 
`SslContextFactory` as the last argument), please double check. 


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171988996
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -154,35 +152,31 @@ public void start() throws Exception {
 
 final boolean authEnabled = 
config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
 
-port = config.getInt(ExecConstants.HTTP_PORT);
-boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
-int retry = 0;
-
-for (; retry < PORT_HUNT_TRIES; retry++) {
-  embeddedJetty = new Server(new 
QueuedThreadPool(config.getInt(ExecConstants.WEB_SERVER_THREAD_POOL_MAX)));
-  embeddedJetty.setHandler(createServletContextHandler(authEnabled));
-  embeddedJetty.addConnector(createConnector(port));
-
+int port = config.getInt(ExecConstants.HTTP_PORT);
+final boolean portHunt = 
config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+final int acceptors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_ACCEPTORS);
+final int selectors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
+final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
+embeddedJetty = new Server(threadPool);
+embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServerConnector connector = createConnector(port, acceptors, 
selectors);
+threadPool.setMaxThreads(1 + connector.getAcceptors() + 
connector.getSelectorManager().getSelectorCount());
+embeddedJetty.addConnector(connector);
+for (int retry = 0; retry < PORT_HUNT_TRIES; retry++) {
+  connector.setPort(port);
   try {
 embeddedJetty.start();
+return;
   } catch (BindException e) {
 if (portHunt) {
-  int nextPort = port + 1;
-  logger.info("Failed to start on port {}, trying port {}", port, 
nextPort);
-  port = nextPort;
-  embeddedJetty.stop();
+  logger.info("Failed to start on port {}, trying port {}", port, 
++port, e);
--- End diff --

I see thx


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171988826
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -411,6 +404,7 @@ private ServerConnector createHttpsConnector(int port) 
throws Exception {
 
 // SSL Connector
 final ServerConnector sslConnector = new ServerConnector(embeddedJetty,
+null, null, null, -1, -1,
--- End diff --

good catch 👍 


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171988104
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -411,6 +404,7 @@ private ServerConnector createHttpsConnector(int port) 
throws Exception {
 
 // SSL Connector
 final ServerConnector sslConnector = new ServerConnector(embeddedJetty,
+null, null, null, -1, -1,
--- End diff --

Why not use the configured values for HTTPS?


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171987969
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -424,10 +418,11 @@ private ServerConnector createHttpsConnector(int 
port) throws Exception {
* @return Initialized {@link ServerConnector} instance for HTTP 
connections.
* @throws Exception
*/
-  private ServerConnector createHttpConnector(int port) throws Exception {
+  private ServerConnector createHttpConnector(int port, int acceptors, int 
selectors) throws Exception {
 logger.info("Setting up HTTP connector for web server");
 final HttpConfiguration httpConfig = new HttpConfiguration();
-final ServerConnector httpConnector = new 
ServerConnector(embeddedJetty, new HttpConnectionFactory(httpConfig));
+final ServerConnector httpConnector =
+new ServerConnector(embeddedJetty, null, null, null, acceptors, 
selectors, new HttpConnectionFactory(httpConfig));
--- End diff --

Why not
 
```
new ServerConnector(embeddedJetty, acceptors, selectors, new 
HttpConnectionFactory(httpConfig));
```


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171987801
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -154,35 +152,31 @@ public void start() throws Exception {
 
 final boolean authEnabled = 
config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
 
-port = config.getInt(ExecConstants.HTTP_PORT);
-boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
-int retry = 0;
-
-for (; retry < PORT_HUNT_TRIES; retry++) {
-  embeddedJetty = new Server(new 
QueuedThreadPool(config.getInt(ExecConstants.WEB_SERVER_THREAD_POOL_MAX)));
-  embeddedJetty.setHandler(createServletContextHandler(authEnabled));
-  embeddedJetty.addConnector(createConnector(port));
-
+int port = config.getInt(ExecConstants.HTTP_PORT);
+final boolean portHunt = 
config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+final int acceptors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_ACCEPTORS);
+final int selectors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
+final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
+embeddedJetty = new Server(threadPool);
+embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServerConnector connector = createConnector(port, acceptors, 
selectors);
+threadPool.setMaxThreads(1 + connector.getAcceptors() + 
connector.getSelectorManager().getSelectorCount());
+embeddedJetty.addConnector(connector);
+for (int retry = 0; retry < PORT_HUNT_TRIES; retry++) {
+  connector.setPort(port);
   try {
 embeddedJetty.start();
+return;
   } catch (BindException e) {
 if (portHunt) {
-  int nextPort = port + 1;
-  logger.info("Failed to start on port {}, trying port {}", port, 
nextPort);
-  port = nextPort;
-  embeddedJetty.stop();
+  logger.info("Failed to start on port {}, trying port {}", port, 
++port, e);
--- End diff --

The QueuedThreadPool is now provided and will be reused. Please see line 
159.


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/1148#discussion_r171986550
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java 
---
@@ -154,35 +152,31 @@ public void start() throws Exception {
 
 final boolean authEnabled = 
config.getBoolean(ExecConstants.USER_AUTHENTICATION_ENABLED);
 
-port = config.getInt(ExecConstants.HTTP_PORT);
-boolean portHunt = config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
-int retry = 0;
-
-for (; retry < PORT_HUNT_TRIES; retry++) {
-  embeddedJetty = new Server(new 
QueuedThreadPool(config.getInt(ExecConstants.WEB_SERVER_THREAD_POOL_MAX)));
-  embeddedJetty.setHandler(createServletContextHandler(authEnabled));
-  embeddedJetty.addConnector(createConnector(port));
-
+int port = config.getInt(ExecConstants.HTTP_PORT);
+final boolean portHunt = 
config.getBoolean(ExecConstants.HTTP_PORT_HUNT);
+final int acceptors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_ACCEPTORS);
+final int selectors = 
config.getInt(ExecConstants.HTTP_JETTY_SERVER_SELECTORS);
+final QueuedThreadPool threadPool = new QueuedThreadPool(2, 2, 6);
+embeddedJetty = new Server(threadPool);
+embeddedJetty.setHandler(createServletContextHandler(authEnabled));
+ServerConnector connector = createConnector(port, acceptors, 
selectors);
+threadPool.setMaxThreads(1 + connector.getAcceptors() + 
connector.getSelectorManager().getSelectorCount());
+embeddedJetty.addConnector(connector);
+for (int retry = 0; retry < PORT_HUNT_TRIES; retry++) {
+  connector.setPort(port);
   try {
 embeddedJetty.start();
+return;
   } catch (BindException e) {
 if (portHunt) {
-  int nextPort = port + 1;
-  logger.info("Failed to start on port {}, trying port {}", port, 
nextPort);
-  port = nextPort;
-  embeddedJetty.stop();
+  logger.info("Failed to start on port {}, trying port {}", port, 
++port, e);
--- End diff --

Originally I stopped the jetty server in the event that a BindException was 
throw since it looks like jetty starts it's beans before it starts its 
connectors (see org.eclipse.jetty.server.Server#doStart). Since the thread pool 
is treated like a bean by Jetty this means the ThreadPool is started before the 
connectors are started. So if a connector fails with a BindException then we 
will have a leaked QueuedThreadPool. My understanding is if we want to release 
resources created during a failed start we would have to stop the jetty server 
after a failure.


---

[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...

[vrozov]

GitHub user vrozov opened a pull request:

https://github.com/apache/drill/pull/1148

DRILL-5994: enable configuring number of Jetty acceptors and selectors 
(default to 1 acceptor and 2 selectors)

@arina-ielchiieva @ilooner @MitchelLabonte Please review

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/vrozov/drill DRILL-5994

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1148.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1148


commit d4feca33e71218905ac5ea1aad55c0b77af2c305
Author: Vlad Rozov 
Date:   2018-03-02T18:39:31Z

DRILL-5994: enable configuring number of Jetty acceptors and selectors 
(default to 1 acceptor and 2 selectors)




---