[GitHub] [nifi] exceptionfactory commented on a change in pull request #5130: NIFI-8639: Add custom headers and dynamic URLs to ConnectWebsocket

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

exceptionfactory commented on a change in pull request #5130:
URL: https://github.com/apache/nifi/pull/5130#discussion_r646963775



##########
File path: nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-processors/pom.xml
##########
@@ -52,5 +52,17 @@
             <version>1.14.0-SNAPSHOT</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.apache.nifi</groupId>
+            <artifactId>nifi-websocket-services-jetty</artifactId>
+            <version>1.14.0-SNAPSHOT</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.apache.nifi</groupId>
+            <artifactId>nifi-websocket-services-jetty</artifactId>
+            <version>1.14.0-SNAPSHOT</version>
+            <scope>test</scope>
+        </dependency>

Review comment:
       This dependency declaration appears to be duplicated.

##########
File path: nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-processors/src/main/java/org/apache/nifi/processors/websocket/AbstractWebSocketGatewayProcessor.java
##########
@@ -165,27 +177,36 @@ private void deregister() {
     }
 
     @Override
-    public final void onTrigger(final ProcessContext context, final ProcessSessionFactory sessionFactory) throws ProcessException {
+    public final void onTrigger(final ProcessContext context, final ProcessSessionFactory sessionFactory) {
         if (processSessionFactory == null) {
             processSessionFactory = sessionFactory;
         }
 
         if (!isProcessorRegisteredToService()) {
             try {
-                registerProcessorToService(context, webSocketService -> onWebSocketServiceReady(webSocketService));
-            } catch (IOException|WebSocketConfigurationException e) {
+                registerProcessorToService(context, webSocketService -> onWebSocketServiceReady(webSocketService, context));
+            } catch (IOException | WebSocketConfigurationException e) {
                 // Deregister processor if it failed so that it can retry next onTrigger.
                 deregister();
                 context.yield();
                 throw new ProcessException("Failed to register processor to WebSocket service due to: " + e, e);
             }
+
+        } else {
+            try {
+                onWebSocketServiceReady(webSocketService, context);
+            } catch (IOException e) {
+                deregister();
+                context.yield();
+                throw new ProcessException("Failed to renew session and connect to WebSocket service due to: " + e, e);

Review comment:
       Should the message include `e.getMessage()` instead of implicitly calling `e.toString()`?
   ```suggestion
                   throw new ProcessException("Failed to renew session and connect to WebSocket service due to: " + e.getMessage(), e);
   ```

##########
File path: nifi-nar-bundles/nifi-websocket-bundle/nifi-websocket-services-jetty/src/main/java/org/apache/nifi/websocket/jetty/JettyWebSocketClient.java
##########
@@ -293,35 +303,37 @@ private void connect(final String clientId, String sessionId) throws IOException
             try {
                 router = routers.getRouterOrFail(clientId);
             } catch (WebSocketConfigurationException e) {
-                throw new IllegalStateException("Failed to get router due to: "  + e, e);
+                throw new IllegalStateException("Failed to get router due to: " + e, e);

Review comment:
       Should this message call `e.getMessage()`?
   ```suggestion
                   throw new IllegalStateException("Failed to get router due to: " + e.getMessage(), e);
   ```




-- 
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