[GitHub] [nifi] turcsanyip commented on a diff in pull request #6540: NIFI-10656 Log ignored event with info instead of warning

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

turcsanyip commented on code in PR #6540:
URL: https://github.com/apache/nifi/pull/6540#discussion_r1026823892


##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureDataLakeStorage.java:
##########
@@ -220,18 +213,25 @@ static void uploadContent(DataLakeFileClient fileClient, InputStream in, long le
     }
 
     //Visible for testing
-    DataLakeFileClient renameFile(final String fileName, final String directoryPath, final DataLakeFileClient fileClient, final boolean overwrite) {
+    String renameFile(final DataLakeFileClient sourceFileClient, final String destinationDirectory, final String destinationFileName, final String conflictResolution) {
         try {
             final DataLakeRequestConditions destinationCondition = new DataLakeRequestConditions();
-            if (!overwrite) {
+            if (!conflictResolution.equals(REPLACE_RESOLUTION)) {
                 destinationCondition.setIfNoneMatch("*");
             }
-            final String destinationPath = createPath(directoryPath, fileName);
-            return fileClient.renameWithResponse(null, destinationPath, null, destinationCondition, null, null).getValue();
+            final String destinationPath = createPath(destinationDirectory, destinationFileName);
+            return sourceFileClient.renameWithResponse(null, destinationPath, null, destinationCondition, null, null).getValue().getFileUrl();
         } catch (DataLakeStorageException dataLakeStorageException) {
-            getLogger().error("Renaming File [{}] failed", fileClient.getFileName(), dataLakeStorageException);
-            removeTempFile(fileClient);
-            throw dataLakeStorageException;
+            removeTempFile(sourceFileClient);
+            if (dataLakeStorageException.getStatusCode() == 409 && conflictResolution.equals(IGNORE_RESOLUTION)) {
+                getLogger().info("File with the same name [{}] already exists. Remote file not modified due to {} being set to '{}'.",
+                        sourceFileClient.getFileName(), CONFLICT_RESOLUTION.getDisplayName(), conflictResolution);
+                return null;
+            } else {
+                // FAIL_RESOLUTION
+                getLogger().error("Renaming File [{}] failed", sourceFileClient.getFileName(), dataLakeStorageException);

Review Comment:
   The error is logged (and therefore appears on the bulletin) twice. It would be enough to log the error in "main" catch block in `onTrigger()`.



##########
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/storage/ITPutAzureDataLakeStorage.java:
##########
@@ -284,17 +285,18 @@ public void testPutFileButFailedToRename() {
         final String componentId = "componentId";
         final DataLakeFileClient fileClient = mock(DataLakeFileClient.class);
         final Response<DataLakeFileClient> response = mock(Response.class);
+        final DataLakeStorageException exception = mock(DataLakeStorageException.class);
         //Mock logger
         when(initContext.getIdentifier()).thenReturn(componentId);
         MockComponentLog componentLog = new MockComponentLog(componentId, processor);
         when(initContext.getLogger()).thenReturn(componentLog);
         processor.initialize(initContext);
         //Mock renameWithResponse Azure method
         when(fileClient.renameWithResponse(isNull(), anyString(), isNull(), any(DataLakeRequestConditions.class), isNull(), isNull())).thenReturn(response);
-        when(response.getValue()).thenThrow(DataLakeStorageException.class);
         when(fileClient.getFileName()).thenReturn(FILE_NAME);
-
-        assertThrows(DataLakeStorageException.class, () -> processor.renameFile(FILE_NAME, "", fileClient, false));
+        when(exception.getStatusCode()).thenReturn(405);
+        when(response.getValue()).thenThrow(exception);
+        assertThrows(DataLakeStorageException.class, () -> processor.renameFile(fileClient, "", FILE_NAME, FAIL_RESOLUTION));

Review Comment:
   If I understand correctly, here we test the "other" error case (non 409 / non-conflicting). Generic errors should be handled the same way in all conflict resolution strategies so the test should be extended with those too:
   ```suggestion
           when(exception.getStatusCode()).thenReturn(405);
           when(response.getValue()).thenThrow(exception);
           assertThrows(DataLakeStorageException.class, () -> processor.renameFile(fileClient, "", FILE_NAME, FAIL_RESOLUTION));
           assertThrows(DataLakeStorageException.class, () -> processor.renameFile(fileClient, "", FILE_NAME, REPLACE_RESOLUTION));
           assertThrows(DataLakeStorageException.class, () -> processor.renameFile(fileClient, "", FILE_NAME, IGNORE_RESOLUTION));
           verify(fileClient, times(3)).delete();
   ```
   
   This and the previous one are unit tests. Not clear why they were added in the IT test class originally. It may be worth moving them to a separate unit test.



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

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org