NIFI-3402: Added etag support to InvokeHTTP #2150

m-hogue · 2017-09-12T18:54:45Z

Thank you for submitting a contribution to Apache NiFi.

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 master)?
Is your initial contribution a single, squashed commit?

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?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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 travis-ci for build issues and submit an update to your PR as soon as possible.

m-hogue · 2017-09-12T18:58:20Z

Since there's not a clean way to unit test this, the way i tested was to change unit tests and observe that there were etag cache hits until max-age=1 seconds had passed, when a network request would be issued again. For example:

2751 [pool-2-thread-1] DEBUG org.apache.nifi.processors.standard.InvokeHTTP - InvokeHTTP[id=6a72be7c-560d-4bdf-b58e-a59aaaec7478] OkHttp ETag cache metrics :: Request Count: 99 | Network Count: 2 | Hit Count: 97

MikeThomsen

Just some minor fixes to follow some of the established best practices for configurability. Other than that, looks like a solid commit to me.

MikeThomsen · 2017-10-25T11:17:14Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

+     * The maximum size, in bytes, that the ETag cache should grow if it is enabled.
+     * The size here is chosen arbitrarily, as the documentation doesn't suggest a recommended value.
+     */
+    private static final int MAX_ETAG_CACHE_SIZE_BYTES = 1024 * 1024 * 10; // 10MiB


Making this configurable with a property descriptor would be a good idea. I think two property descriptors, one for the numeric portion and the other for the unit would be very helpful to users.

MikeThomsen · 2017-10-25T11:18:11Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

@@ -669,6 +694,14 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
        final int maxAttributeSize = context.getProperty(PROP_PUT_ATTRIBUTE_MAX_LENGTH).asInteger();
        final ComponentLog logger = getLogger();

+        // log ETag cache metrics
+        final boolean eTagEnabled = context.getProperty(PROP_USE_ETAG).asBoolean();
+        if(eTagEnabled) {


I think && logger.isDebugEnabled() should be added to this line to prevent the extra work being done since this is only for debugging.

MikeThomsen · 2017-10-25T11:19:16Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

+    public static final PropertyDescriptor PROP_USE_ETAG = new PropertyDescriptor.Builder()
+            .name("use-etag")
+            .displayName("Use HTTP ETag")
+            .required(true)


This needs a description("") call to set a description statement on the property.

m-hogue · 2017-10-25T14:52:46Z

Thanks for the review, @MikeThomsen! I agree & I'll make all of these changes.

…-etag property

m-hogue · 2017-10-25T15:49:07Z

@MikeThomsen : I've added your suggestions. Please let me know if you'd like any more changes.

MikeThomsen · 2017-10-31T13:19:39Z

Looks good to me. The only change I'd recommend is do a squashed commit so your patch is just one commit.

Not a core committer, but FWIW +1

pvillard31 · 2017-10-31T16:48:10Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

+     * @return the directory in which the ETag cache should be written
+     */
+    private static File getETagCacheDir() {
+        return Files.createTempDir();


Should this directory be deleted on @OnUnscheduled annotation call? Or is this done automatically?

As it's currently written, this should be done automatically via OS-controlled temporary folder cleanup [1]. I elected to do this to 1) avoid manual cleanup and 2) avoid introducing something error prone. As this is only a cache, its durability isn't important.

[1] https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Files.html#createTempDir()

I think it's a good way to go. It should be platform agnostic and work as a sane default.

Fair enough. I just thought that OS-controlled temporary folder was being cleaned at server reboot by default and a NiFi server is probably not supposed to be rebooted very often.

@pvillard31 Since there is a configured size limit on the field, it should stick to a reasonable size since there is a sane default of 10MB. If a user is foolish enough to set that to 10GB, not our problem.

@MikeThomsen - true but one temporary folder is created each time the processor is started. We could assume that the processor is going to be stopped/started a lot. But I agree, this is a edge case. I'm fine with the current implementation. If I really want to be meticulous I could suggest to add this information in the property description :)

MikeThomsen · 2018-03-13T01:33:45Z

@pvillard31 @m-hogue Think we can close the loop on this one?

pvillard31 · 2018-03-13T09:21:26Z

Yes. +1, merging to master, thanks!

MikeThomsen requested changes Oct 25, 2017

View reviewed changes

m-hogue added 3 commits October 25, 2017 11:37

NIFI-3402: Added etag support to InvokeHTTP

7faa61c

NIFI-3402: Added configurable max etag cache size, description to use…

cdf7ff5

…-etag property

NIFI-3402: Moved isDebugEnabled() check to appropriate spot.

5d2309a

pvillard31 reviewed Oct 31, 2017

View reviewed changes

pvillard31 approved these changes Mar 13, 2018

View reviewed changes

asfgit closed this in 6471f66 Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-3402: Added etag support to InvokeHTTP #2150

NIFI-3402: Added etag support to InvokeHTTP #2150

m-hogue commented Sep 12, 2017 •

edited

m-hogue commented Sep 12, 2017

MikeThomsen left a comment

MikeThomsen Oct 25, 2017

MikeThomsen Oct 25, 2017

MikeThomsen Oct 25, 2017

m-hogue commented Oct 25, 2017

m-hogue commented Oct 25, 2017

MikeThomsen commented Oct 31, 2017

pvillard31 Oct 31, 2017

m-hogue Nov 6, 2017 •

edited

MikeThomsen Nov 6, 2017

pvillard31 Nov 7, 2017

MikeThomsen Nov 7, 2017

pvillard31 Nov 7, 2017

MikeThomsen commented Mar 13, 2018

pvillard31 commented Mar 13, 2018

NIFI-3402: Added etag support to InvokeHTTP #2150

NIFI-3402: Added etag support to InvokeHTTP #2150

Conversation

m-hogue commented Sep 12, 2017 • edited

For all changes:

For code changes:

For documentation related changes:

Note:

m-hogue commented Sep 12, 2017

MikeThomsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-hogue commented Oct 25, 2017

m-hogue commented Oct 25, 2017

MikeThomsen commented Oct 31, 2017

Choose a reason for hiding this comment

m-hogue Nov 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeThomsen commented Mar 13, 2018

pvillard31 commented Mar 13, 2018

m-hogue commented Sep 12, 2017 •

edited

m-hogue Nov 6, 2017 •

edited