You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2016/04/08 12:05:14 UTC
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/337
NIFI-1732 Added timeout property to override 30s default when handling HTTP requests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pvillard31/nifi NIFI-1732
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/337.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 #337
----
commit 50ee6b9e619212424dd29b9b642a088356b343ca
Author: Pierre Villard <pi...@gmail.com>
Date: 2016-04-08T10:04:39Z
NIFI-1732 Added timeout property to override 30s default when handling HTTP requests
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the pull request:
https://github.com/apache/nifi/pull/337#issuecomment-217470408
@pvillard31 According to the SO linked in the ticket, when the timeout being set is reached "the request is redispatch with a DispatcherType of ASYNC". In StandardHttpContextMap there already is a "Request Expiration" property. This will remove the request from the HttpContextMap's internal queue and respond with a "SERVICE UNAVAILABLE" once the Expiration ("timeout") hits[2].
This leads to the question: does the user really want to respond in this Jetty specific way or should the timeout of jetty just be set higher than the ContextMap Expiration so that the request responds with "SERVICE UNAVAILABLE"?
[1] https://github.com/apache/nifi/blob/aa99884782e54c54ee138f5609b3be84628e96f9/nifi-nar-bundles/nifi-standard-services/nifi-http-context-map-bundle/nifi-http-context-map/src/main/java/org/apache/nifi/http/StandardHttpContextMap.java#L59-L59
[2] https://github.com/apache/nifi/blob/aa99884782e54c54ee138f5609b3be84628e96f9/nifi-nar-bundles/nifi-standard-services/nifi-http-context-map-bundle/nifi-http-context-map/src/main/java/org/apache/nifi/http/StandardHttpContextMap.java#L178-L178
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/337#issuecomment-217500797
Thanks @JPercivall
Few remarks:
- when there is a timeout, what I observed is a request with a dispatcher type set to ERROR (I've never seen ASYNC so far).
- I was not aware of the cleaning job in the context map, but since the jetty timeout is 30 seconds, that would explain why the context map is never cleared once jetty starts to continuously send error dispatch messages.
I am definitely a +1 to remove the property and automatically set the timeout in line with the context map. It'll prevent strange behavior, and as I said in the JIRA, if the timeout is not correctly set by the user, we would potentially see the context map filled after a moment. I'll update the PR accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/337#discussion_r59014799
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -220,6 +221,13 @@
.allowableValues(CLIENT_NONE, CLIENT_WANT, CLIENT_NEED)
.defaultValue(CLIENT_NONE.getValue())
.build();
+ public static final PropertyDescriptor TIMEOUT = new PropertyDescriptor.Builder()
+ .name("Timeout")
+ .description("The timeout to set when handling requests")
--- End diff --
Can you elaborate on exactly what happens when this timeout expires? Is an HTTP Response sent to the client? Does the request timeout on the client side, etc.?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/337#discussion_r59015792
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -220,6 +221,13 @@
.allowableValues(CLIENT_NONE, CLIENT_WANT, CLIENT_NEED)
.defaultValue(CLIENT_NONE.getValue())
.build();
+ public static final PropertyDescriptor TIMEOUT = new PropertyDescriptor.Builder()
+ .name("Timeout")
+ .description("The timeout to set when handling requests")
--- End diff --
@markap14 I believe this is something specific to jetty. As reported on the mailing list/JIRA, when the timeout expires, there is no response sent to the client, instead there is a new request received in the processor with some differences:
> the http.context.identifier and the FlowFile UUID has changed, and the http.dispatcher.type has changed from REQUEST to ERROR
If the timeout is not set properly (or if there is a problem in the flow and response is never sent), it could generate an infinite loop leading to the problem reported
> If I then leave the system alone for much longer than the Request Timeout parameter in the StandardHttpContextMap and then attempt a request, I get a 503 response saying that the queue is full and no requests are allowed. No requests are allowed at all until I delete and recreate the Map.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on the pull request:
https://github.com/apache/nifi/pull/337#issuecomment-217590465
Whops forgot to comment before pushing:
+1
Just sets the jetty time out to Long.Max_Value so that the built in timeout feature will be the go to. Pushed to master and 0.x
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/337#discussion_r59014373
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -220,6 +221,13 @@
.allowableValues(CLIENT_NONE, CLIENT_WANT, CLIENT_NEED)
.defaultValue(CLIENT_NONE.getValue())
.build();
+ public static final PropertyDescriptor TIMEOUT = new PropertyDescriptor.Builder()
+ .name("Timeout")
+ .description("The timeout to set when handling requests")
+ .required(false)
+ .defaultValue("30s")
--- End diff --
We typically would use a format of "30 sec" for the default. Just to try to keep things consistent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/337#issuecomment-207403440
@markap14 Thanks for the comments, I updated the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/337
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/337#discussion_r59014716
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -270,6 +279,9 @@ private synchronized void initializeServer(final ProcessContext context) throws
final int port = context.getProperty(PORT).asInteger();
final SSLContextService sslService = context.getProperty(SSL_CONTEXT).asControllerService(SSLContextService.class);
+ final String rawTimeout = context.getProperty(TIMEOUT).getValue();
--- End diff --
These two lines can be simplified to:
{code}
final long timeout = context.getProperty(TIMEOUT).asTimePeriod(TimeUnit.MILLISECONDS);
{code}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1732 Added timeout property to override 30...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/337#discussion_r59014646
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -220,6 +221,13 @@
.allowableValues(CLIENT_NONE, CLIENT_WANT, CLIENT_NEED)
.defaultValue(CLIENT_NONE.getValue())
.build();
+ public static final PropertyDescriptor TIMEOUT = new PropertyDescriptor.Builder()
+ .name("Timeout")
+ .description("The timeout to set when handling requests")
+ .required(false)
--- End diff --
I typically recommend setting required = true if there is a default value. This is just because a value will always be present and as a result this flag doesn't really mean a whole lot. So I use required = true for consistency.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---