[GitHub] drill pull request #1148: DRILL-5994: enable configuring number of Jetty acc...
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...
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...
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...
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...
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...
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...
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...
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...
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) ---