[GitHub] [nifi] jfrazee commented on a change in pull request #4183: NIFI-7176 - InvokeHTTP - timeout supported expression language

[GitBox <gi...@apache.org>]

jfrazee commented on a change in pull request #4183:
URL: https://github.com/apache/nifi/pull/4183#discussion_r580569383



##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -1268,4 +1284,13 @@ public boolean verify(String hostname, SSLSession session) {
             return delegate.verify(hostname, session);
         }
     }
-}
+
+    class TimeoutInterceptor implements Interceptor {
+        @NonNull
+        @Override
+        public Response intercept(@NonNull Chain chain) throws IOException {
+            Chain chainWithNewTimeout = chain.withReadTimeout(timeout, TimeUnit.MILLISECONDS);
+            return chainWithNewTimeout.proceed(chain.request());
+        }
+    }
+}

Review comment:
       Can you make sure there's a newline here?

##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -788,6 +801,9 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
             }
         }
 
+        if (context.getProperty(PROP_READ_TIMEOUT).isExpressionLanguagePresent())
+            timeout = context.getProperty(PROP_READ_TIMEOUT).evaluateAttributeExpressions(requestFlowFile).asTimePeriod(TimeUnit.MILLISECONDS).intValue();

Review comment:
       @guvencenanguvenal As far as I can tell the static shared timeout is going to create a concurrency issue. I think we should be using `newBuilder()` here instead of an interceptor (see [here](https://square.github.io/okhttp/3.x/okhttp/okhttp3/OkHttpClient.html) and [here](https://github.com/square/okhttp/blob/b3dcb9b1871325248fba917458658628c44ce8a3/docs/recipes.md#per-call-configuration-kt-java)). For example:
   ```suggestion
           if (context.getProperty(PROP_READ_TIMEOUT).isExpressionLanguagePresent()) {
               final int connectTimeout = context.getProperty(PROP_CONNECT_TIMEOUT).evaluateAttributeExpressions(requestFlowFile).asTimePeriod(TimeUnit.MILLISECONDS).intValue();        
               final int readTimeout = context.getProperty(PROP_READ_TIMEOUT).evaluateAttributeExpressions(requestFlowFile).asTimePeriod(TimeUnit.MILLISECONDS).intValue();         
               okHttpClient = okHttpClient.newBuilder()
                       .connectTimeout(connectTimeout, TimeUnit.MILLISECONDS)            
                       .readTimeout(readTimeout, TimeUnit.MILLISECONDS)
                       .build();
           }
   ```

##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -638,8 +637,12 @@ public void setUpClient(final ProcessContext context) throws IOException, Unreco
         }
 
         // Set timeouts
-        okHttpClientBuilder.connectTimeout((context.getProperty(PROP_CONNECT_TIMEOUT).asTimePeriod(TimeUnit.MILLISECONDS).intValue()), TimeUnit.MILLISECONDS);
-        okHttpClientBuilder.readTimeout(context.getProperty(PROP_READ_TIMEOUT).asTimePeriod(TimeUnit.MILLISECONDS).intValue(), TimeUnit.MILLISECONDS);
+        okHttpClientBuilder.connectTimeout((context.getProperty(PROP_CONNECT_TIMEOUT).evaluateAttributeExpressions().asTimePeriod(TimeUnit.MILLISECONDS).intValue()), TimeUnit.MILLISECONDS);
+
+        if (context.getProperty(PROP_READ_TIMEOUT).isExpressionLanguagePresent())
+            okHttpClientBuilder.addInterceptor(new TimeoutInterceptor());

Review comment:
       See my comment on `newBuilder()` bewlow. I think it's right that it's not sufficient to set it on the primary builder b/c you want to use per-flowfile attributes, but I think the static timeout + an interceptor is problematic.

##########
File path: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java
##########
@@ -66,6 +66,8 @@
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
 import javax.net.ssl.X509TrustManager;
+
+

Review comment:
       ```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