exceptionfactory commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583893378
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestListenHTTP.java
##########
@@ -405,6 +409,84 @@ public void
testSecureServerRejectsUnsupportedTlsProtocolVersion() throws Except
assertThrows(SSLHandshakeException.class, sslSocket::startHandshake);
}
+ @Test
+ public void testMaxThreadPoolSizeTooLow() {
+ // GIVEN, WHEN
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE, "7");
+
+ // THEN
+ runner.assertNotValid();
+ }
+
+ @Test
+ public void testMaxThreadPoolSizeTooHigh() {
+ // GIVEN, WHEN
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE, "1001");
+
+ // THEN
+ runner.assertNotValid();
+ }
+
+ @Test
+ public void testMaxThreadPoolSizeOkLowerBound() {
+ // GIVEN, WHEN
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE, "8");
+
+ // THEN
+ runner.assertValid();
+ }
+
+ @Test
+ public void testMaxThreadPoolSizeOkUpperBound() {
+ // GIVEN, WHEN
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE, "1000");
+
+ // THEN
+ runner.assertValid();
+ }
+
+ @Test
+ public void
testWhenServerIsStartedCreateQueuedThreadPoolIsCalledWithMaxThreadPoolSize() {
+ // GIVEN
+ int maxThreadPoolSize = 200;
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE,
Integer.toString(maxThreadPoolSize));
+
+ // WHEN
+ startWebServer();
+
+ // THEN
+ Mockito.verify(proc).createQueuedThreadPool(maxThreadPoolSize);
+ }
+
+ @Test
+ public void
testWhenServerIsStartedCreateCreateServerIsCalledWithTheRightQueuedThreadPool()
{
+ // GIVEN
+ int maxThreadPoolSize = 200;
+ QueuedThreadPool queuedThreadPool = new
QueuedThreadPool(maxThreadPoolSize);
+
+ runner.setProperty(ListenHTTP.PORT, Integer.toString(availablePort));
+ runner.setProperty(ListenHTTP.BASE_PATH, HTTP_BASE_PATH);
+ runner.setProperty(ListenHTTP.MAX_THREAD_POOL_SIZE,
Integer.toString(maxThreadPoolSize));
+
+
Mockito.when(proc.createQueuedThreadPool(maxThreadPoolSize)).thenReturn(queuedThreadPool);
Review comment:
Instead of exposing `createThreadPool()` as a protected method for
testing, it should only be necessary to expose `createServer()` as protected.
The Jetty `Server` class has a `getThreadPool()` method which can then be used
for validation. The return interface `ThreadPool` is generic, but it is
possible to check that it is an instance of the public `SizedThreadPool`
interface and then compare the results of `getMaxThreads()`. That would remove
the need to use the Mockito `Spy` annotation on the processor and allow for a
simple `assertEquals(maxThreadPoolSize, sizedThreadPool.getMaxThreads())`
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -226,6 +226,20 @@ public AllowableValue getAllowableValue() {
.defaultValue(ClientAuthentication.AUTO.name())
.dependsOn(SSL_CONTEXT_SERVICE)
.build();
+ public static final PropertyDescriptor MAX_THREAD_POOL_SIZE = new
PropertyDescriptor.Builder()
+ .name("max-thread-pool-size")
+ .displayName("Maximum Thread Pool Size")
+ .description("The maximum number of threads to be used by the
embedded Jetty server. "
+ + "The value can be set between 8 and 1000. "
+ + "The value of this property affects the performance of
the flows and the operating system, therefore "
+ + "the default value should only be changed in justified
cases. "
+ + "A value that is less than the default value may be
suitable "
+ + "if only a small number of HTTP clients connect to the
server. A greater value may be suitable "
+ + "if a large number of HTTP clients are expected to make
requests to the server simultaneously.")
+ .required(true)
+ .addValidator(StandardValidators.createLongValidator(8L, 1000L,
true))
Review comment:
The default of value of 200 threads is clear in the Jetty Server class,
but it wasn't clear as to what drives these minimum and maximum values. Is a
minimum value of `1` supported? Does Jetty allow specifying more than 1000
threads?
----------------------------------------------------------------
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:
[email protected]
