You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/02/25 17:36:40 UTC
[GitHub] [nifi] pgyori opened a new pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
pgyori opened a new pull request #4847:
URL: https://github.com/apache/nifi/pull/4847
https://issues.apache.org/jira/browse/NIFI-8263
#### Description of PR
Introduces Maximum Thread Pool Size property in the ListenHTTP processor to be able to set the maximum number of threads used by the Jetty server.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as 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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [nifi] pgyori commented on pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pgyori commented on pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#issuecomment-786865501
Thank you @exceptionfactory !
You are right, there is no simple way to check the thread pool size within the Server instance. After your comment, I thought about it and found that we do not need to check the server's thread pool size as long as we can test what value the server's constructor is called with. So I organized the server instantiation into a separate method and created unit tests for it. My modifications are in the recent commit.
----------------------------------------------------------------
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583916702
##########
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:
Thanks for providing that background @pgyori, the lower boundary makes sense given the behavior of `QueuedThreadPool`. Having `1000` as the upper boundary sounds like a reasonable limit since more is not always better when it comes to threads. With that explanation and the unit tests, the PR looks good!
----------------------------------------------------------------
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
[GitHub] [nifi] pgyori commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pgyori commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583907815
##########
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:
I don't think we need to test whether the Server(ThreadPool pool) constructor works correctly, because that should be covered in the Jetty server's unit tests. All we need to make sure is that the constructor is called with the right parameter.
The other thing is that the server creation is called from onTrigger. I believe that what we need to test here is not that the server creator method can correctly create a server (in isolation) with the right thread pool size, but that when the processor is started, server creation is executed with the right parameter. That is why I called startWebServer() in the unit test, which calls onTrigger() in which all the server initialization happens, and I needed the Spy to make sure that the createServer() method is called with the right parameter object. This way, if someone removes my modification from the code by replacing the new:
final Server server = createServer(threadPool);
with the old:
final Server server = new Server(threadPool);
the unit test will fail.
----------------------------------------------------------------
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
[GitHub] [nifi] pgyori commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pgyori commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r584753765
##########
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:
@exceptionfactory
I had a look at it with fresh eyes. With your idea, and the fact that the server itself is stored as an instance field in ListenHTTP (which is set at the end of the createHttpServerFromService() method), I managed to simplify the test. This way there is no need for the protected methods in ListenHTTP (instead, a package-private method needed to be introduced to get the Server instance).
This still tests everything I wrote above, but without needing a Spy object.
Thank you for your suggestion!
----------------------------------------------------------------
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
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583915058
##########
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:
Thanks for the explanation @pgyori, that makes sense.
----------------------------------------------------------------
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
[GitHub] [nifi] pgyori commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pgyori commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583917079
##########
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:
Thank you, @exceptionfactory !
----------------------------------------------------------------
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
[GitHub] [nifi] asfgit closed pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4847:
URL: https://github.com/apache/nifi/pull/4847
----------------------------------------------------------------
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
[GitHub] [nifi] pgyori commented on a change in pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pgyori commented on a change in pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#discussion_r583911727
##########
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:
Yes, a minimum value of 1 is supported. However, the relevant constructor of QueuedThreadPool uses the minimum value of 8 hard coded if the max value is greater than 8. Based on that, I do not think it is advised to set a very low number of threads, to avoid performance issues with the Processor. Also, there is no limit for the max value (except for what an integer can hold), but we want to avoid abuse of this setting because it may lead to performance issues on the entire system NiFi is running on. Although the upper limit I wrote there is up for discussion.
----------------------------------------------------------------
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
[GitHub] [nifi] pvillard31 commented on pull request #4847: NIFI-8263: Maximum Thread Pool Size property introduced in ListenHTTP
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4847:
URL: https://github.com/apache/nifi/pull/4847#issuecomment-787991000
Waiting for the build checks to complete. If all green, will merge.
----------------------------------------------------------------
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