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/03 17:33:45 UTC
[GitHub] [nifi] thenatog opened a new pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
thenatog opened a new pull request #4804:
URL: https://github.com/apache/nifi/pull/4804
…ol for requests.
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] 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:
- [x] 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?
- [x] 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] markap14 commented on pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-774208813
I think I would be in favor of Disable also but don't have a particularly strong opinion.
----------------------------------------------------------------
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 pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-774135748
@thenatog One additional note in favor of reversing the property to "Disable HTTP/2" instead of "Support HTTP2/": Naming a new property "Support HTTP/2" makes it sound like the processor did not previously support HTTP/2, when it actually did. In the end, these are certainly just semantic differences. Although leaning toward "Disable HTTP/2", if you and @markap14 prefer "Support HTTP/2", that's fine.
----------------------------------------------------------------
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] thenatog commented on pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-772718480
I guess it is possible to allow users to explicitly choose a protocol, but they are not mutually exclusive. If you choose HTTP/2, we will need to include HTTP/1.1 as well, which is basically what the True/False enables. I don't know if users are that interested in figuring out what protocol to choose eg. H2_PRIOR_KNOWLEDGE or QUIC.
I can change the default to True. I also forgot to add user docs so I'll do that now.
----------------------------------------------------------------
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 pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-775447609
Thanks for adjusting the property naming approach @thenatog. Built and tested. +1 Merging.
----------------------------------------------------------------
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] thenatog commented on pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-775376092
Updated to Disable HTTP/2 instead.
----------------------------------------------------------------
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 #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#discussion_r569690686
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
+ .required(true)
+ .defaultValue("True")
+ .allowableValues("True", "False")
Review comment:
Thanks for the reply, it looks like this processor has both sets of options, so with that background, the current approach works.
----------------------------------------------------------------
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] thenatog commented on pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-772741138
I've set the default to True and realized the documentation for standard processors is compiled from PropertyDescriptor descriptions which I had already added.
I think that allowing users to explicitly set protocols may be too library specific. "H2_PRIOR_KNOWLEDGE" for example sounds like it is specifically for use with the okhttp client library for certain use cases, and is not necessarily broadly applicable to HTTP generally. I may be wrong.
My approach would be to go with this simpler approach for now, and if we find that users want more specific control then we can move to that. The issue again being that they would be specifying names of protocols that are derived from okhttp which may not exist in a different HTTP client library.
----------------------------------------------------------------
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 #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#discussion_r569680427
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
+ .required(true)
+ .defaultValue("True")
+ .allowableValues("True", "False")
Review comment:
Should Boolean values be lowercase strings to match standard primitive usage and other Boolean properties?
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
Review comment:
Recommend a slight wording change to use `HTTP/2` instead of `HTTP 2` in order to match official naming conventions.
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
Review comment:
Recommend rewording to `Support HTTP/2`
```suggestion
.displayName("Support HTTP/2")
```
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -743,6 +755,12 @@ public void setUpClient(final ProcessContext context) throws TlsException {
okHttpClientBuilder.cache(new Cache(getETagCacheDir(), maxCacheSizeBytes));
}
+ if(context.getProperty(SUPPORT_HTTP2_PROTOCOL).asBoolean()) {
Review comment:
Should there be a space after the `if`?
```suggestion
if (context.getProperty(SUPPORT_HTTP2_PROTOCOL).asBoolean()) {
```
----------------------------------------------------------------
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] thenatog commented on a change in pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
thenatog commented on a change in pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#discussion_r569788882
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
+ .required(true)
+ .defaultValue("True")
+ .allowableValues("True", "False")
Review comment:
Yes it seems like even just within this processor there is both "true" and "True". I'll remove the boolean validator - it seems like some properties have the validator and some don't.
----------------------------------------------------------------
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 #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4804:
URL: https://github.com/apache/nifi/pull/4804
----------------------------------------------------------------
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] markap14 commented on a change in pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
markap14 commented on a change in pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#discussion_r569688751
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
+ .required(true)
+ .defaultValue("True")
+ .allowableValues("True", "False")
Review comment:
Debatable on this :) Typically we favor capitalized words in nifi, but with boolean we've tended to use true/false instead of True/False. There's a good mixture throughout the codebase unfortunately. If there is already at least one boolean property in the processor i would say at least try to stay consistent. Otherwise... 🤷♂️
Of note, though, there's no need for the Boolean Validator, as there are allowable values.
----------------------------------------------------------------
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] thenatog commented on a change in pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
thenatog commented on a change in pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#discussion_r569788882
##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -474,6 +475,16 @@
.allowableValues("true", "false")
.build();
+ public static final PropertyDescriptor SUPPORT_HTTP2_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("support-http2")
+ .description("Determines whether or not to support the HTTP 2 protocol version.")
+ .displayName("Support HTTP 2")
+ .required(true)
+ .defaultValue("True")
+ .allowableValues("True", "False")
Review comment:
Yes it seems like even just within this processor there is both "true" and "True". I'll remove the boolean validator.
----------------------------------------------------------------
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] markap14 commented on pull request #4804: NIFI-8181 - Added a property to specify whether to allow HTTP2 protoc…
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #4804:
URL: https://github.com/apache/nifi/pull/4804#issuecomment-772719837
That's interesting @exceptionfactory. I didn't realize there were other additional protocols supported by the library. Personally I'm a bit torn on how I think we should best approach it. We can have a single property for specifying which protocols to support, but that has the downside that it's a bad user experience. It requires that the user look up the documentation each time to understand exactly what needs to be typed in, and must then type it in whereas the boolean approach allows them to just select the value (when it's appropriate). We could add an additional property for each protocol that that means we'd be adding several properties to a processor that already has *a lot* of properties.
What it really comes down to, IMO, is a trade-off between power vs. ease of use. The boolean property is easier to use when it applies (which I think is going to be > 99% of the time) but is less powerful because it doesn't support the additional protocols. The other options are not as nice a user experience but are more powerful.
Personally, I think we should lean more toward ease of use than power, so I think I prefer the boolean.
----------------------------------------------------------------
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