NIFI-10230 added FetchSmb by kulikg · Pull Request #6279 · apache/nifi

kulikg · 2022-08-09T13:36:08Z

Summary

NIFI-00000

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 8
- [x ] JDK 11
- JDK 17

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

exceptionfactory

Thanks for introducing this new component @kulikg!

The general implementation looks good, I noted a few questions about property configuration and error handling.

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

exceptionfactory · 2022-08-10T17:41:31Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+            REL_FAILURE,
+            REL_INPUT_FAILURE
+    )));
+    static final PropertyDescriptor RECORD_READER = new PropertyDescriptor.Builder()


This property descriptor declaration should be moved up in the file, following after the SMB_CLIENT_PROVIDER_SERVICE.

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

exceptionfactory · 2022-08-10T17:48:07Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+            } catch (Exception e) {
+                handleInputError(e, session, flowFile);
+            } finally {
+                session.remove(flowFile);


It seems like the original FlowFile should be routed on failures, instead of cloning and creating a new FlowFile. Following this approach, the input FlowFile would be removed only on a successful fetch.

exceptionfactory · 2022-08-10T17:49:29Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+        final SmbClientProviderService clientProviderService =
+                context.getProperty(SMB_CLIENT_PROVIDER_SERVICE).asControllerService(SmbClientProviderService.class);
+
+        try (SmbClientService client = clientProviderService.getClient()) {


The current implementation repeatedly calls getClient() when using record-oriented processing. It would be more efficient to move the retrieval of the SmbClientService out of this method.

exceptionfactory · 2022-08-10T17:51:05Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+            session.putAttribute(outFlowFile, ERROR_CODE_ATTRIBUTE, getErrorCode(e));
+            session.putAttribute(outFlowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage());


The return value from putAttribute() needs to be assigned so that the session is working with the most recent FlowFile reference object.

In general, method arguments should be declared as final, but in this case, the outFlowFile variable could be reassigned:

Suggested change

session.putAttribute(outFlowFile, ERROR_CODE_ATTRIBUTE, getErrorCode(e));

session.putAttribute(outFlowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage());

outFlowFile = session.putAttribute(outFlowFile, ERROR_CODE_ATTRIBUTE, getErrorCode(e));

outFlowFile = session.putAttribute(outFlowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage());

I think the test framework should complain about this too.

It might be difficult to implement this correctly, but it could be a useful addition under a separate issue.

exceptionfactory · 2022-08-10T17:52:52Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+
+    private void handleInputError(Exception exception, ProcessSession session, FlowFile flowFile) {
+        if (exception instanceof IOException || exception instanceof MalformedRecordException || exception instanceof SchemaNotFoundException) {
+            getLogger().error("Couldn't read file metadata content as records from incoming flowfile", exception);


Recommend adjusting the wording and including the FlowFile in the log for troubleshooting:

Suggested change

getLogger().error("Couldn't read file metadata content as records from incoming flowfile", exception);

getLogger().error("Failed to read input records {}", flowFile, exception);

exceptionfactory · 2022-08-10T17:54:21Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+        if (exception instanceof IOException || exception instanceof MalformedRecordException || exception instanceof SchemaNotFoundException) {
+            getLogger().error("Couldn't read file metadata content as records from incoming flowfile", exception);
+        } else {
+            getLogger().error("Unexpected error while processing incoming flowfile", exception);


Recommend adjusting the wording and including the FlowFile:

Suggested change

getLogger().error("Unexpected error while processing incoming flowfile", exception);

getLogger().error("Failed to read input {}", flowFile, exception);

exceptionfactory · 2022-08-10T17:56:51Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+        return Optional.ofNullable(exception instanceof SmbException ? (SmbException) exception : null)
+                .map(SmbException::getErrorCode)
+                .map(String::valueOf)
+                .orElse("N/A");


The use of N/A as a placeholder does not seem like the best option. If -1 does not already have special meaning from the SmbException, that is one option. Otherwise, some other negative number seems best, or perhaps this method should return null instead.

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

...ifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/ListSmb.java

...bundle/nifi-smb-client-api/src/main/java/org/apache/nifi/services/smb/SmbListableEntity.java

nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/pom.xml

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

turcsanyip · 2022-08-29T13:16:27Z

@kulikg There are Checkstyle violations. Please always run mvn clean verify -P contrib-check before adding a commit.

turcsanyip

@kulikg Thanks for your latest changes! Please find my new comments below.

...undle/nifi-smb-smbj-client/src/main/java/org/apache/nifi/services/smb/SmbjClientService.java

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

...mb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/FetchSmbTest.java

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

...bundle/nifi-smb-client-api/src/main/java/org/apache/nifi/services/smb/SmbListableEntity.java

...ifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/ListSmb.java

tpalfy · 2022-08-30T18:03:07Z

nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/pom.xml

            <version>1.18.0-SNAPSHOT</version>
            <scope>test</scope>
        </dependency>
+        <dependency>


I think we no longer need this dependency.

tpalfy · 2022-08-30T18:06:28Z

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

+import org.apache.nifi.services.smb.SmbException;
+
+@InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED)
+@Tags({"samba, smb, cifs, files", "fetch"})


Not sure if this is an error - searching based on these tags still work on UI:

Suggested change

@Tags({"samba, smb, cifs, files", "fetch"})

@Tags({"samba", "smb", "cifs", "files", "fetch"})

tpalfy · 2022-08-30T18:20:23Z

...le/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/SambaTestContainers.java

+        sambaContainer.stop();
+    }
+
+    protected SmbjClientProviderService configureTestRunnerForSambaDockerContainer(TestRunner testRunner)


Suggested change

protected SmbjClientProviderService configureTestRunnerForSambaDockerContainer(TestRunner testRunner)

protected SmbjClientProviderService configureSmbClient(TestRunner testRunner)

tpalfy · 2022-08-30T18:21:42Z

...le/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/SambaTestContainers.java

+        testRunner.setProperty(smbjClientProviderService, DOMAIN, "domain");
+        return smbjClientProviderService;


I think it would make sense to enable the controller service here instead of all individual tests.

Suggested change

testRunner.setProperty(smbjClientProviderService, DOMAIN, "domain");

return smbjClientProviderService;

testRunner.setProperty(smbjClientProviderService, DOMAIN, "domain");

testRunner.enableControllerService(smbjClientProviderService);

return smbjClientProviderService;

tpalfy · 2022-08-30T18:33:25Z

...i-smb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/ListSmbIT.java

        allAttributes.forEach(attribute -> assertEquals(
                Stream.of(attribute.get("path"), attribute.get("filename")).filter(s -> !s.isEmpty()).collect(
                        Collectors.joining("/")),


absolute.path attribute is no longer available. This test currently fails. Should remove this assertion.

...bundle/nifi-smb-client-api/src/main/java/org/apache/nifi/services/smb/SmbListableEntity.java

...fi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java

...ifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/ListSmb.java

turcsanyip

@kulikg Thanks for the review changes!

+1 LGTM

tpalfy · 2022-09-06T16:20:31Z

LGTM.

I think it's ready to be merged.
@exceptionfactory, do you have any additional comments?

exceptionfactory

Thanks for checking @tpalfy, and great works on this processor @kulikg!

The latest version looks good, so feel free to merge @tpalfy.

As a minor note, it is best to avoid contractions and trailing periods in log messages, but it is fine to proceed for now.

tpalfy · 2022-09-06T17:01:15Z

Thanks for your work @kulikg!
Thanks for you review @turcsanyip and @exceptionfactory!
Merged to main.

This closes apache#6279. Signed-off-by: Tamas Palfy <tpalfy@apache.org>

NIFI-10230 added FetchSmb

68a2069

exceptionfactory requested changes Aug 10, 2022

View reviewed changes

Gabor Kulik added 3 commits August 17, 2022 14:55

NIFI-10230 review items

f30a378

NIFI-10230 java8 compatiblity

a3e1814

NIFI-10230 review items

9ade91b

turcsanyip requested changes Aug 18, 2022

View reviewed changes

Gabor Kulik added 2 commits August 23, 2022 18:11

NIFI-10230 review items

720667c

NIFI-10230 remove record reader

bb52e13

NIFI-10230 fix checkstyle

f29bfbd

turcsanyip requested changes Aug 29, 2022

View reviewed changes

turcsanyip requested changes Aug 30, 2022

View reviewed changes

NIFI-10230 review items

2fcc79b

tpalfy reviewed Aug 30, 2022

View reviewed changes

turcsanyip requested changes Sep 1, 2022

View reviewed changes

NIFI-10230 review items

c4164b5

turcsanyip approved these changes Sep 3, 2022

View reviewed changes

exceptionfactory approved these changes Sep 6, 2022

View reviewed changes

asfgit closed this in 5b56567 Sep 6, 2022

p-kimberley pushed a commit to p-kimberley/nifi that referenced this pull request Oct 15, 2022

NIFI-10230 added FetchSmb

f7367b4

This closes apache#6279. Signed-off-by: Tamas Palfy <tpalfy@apache.org>

		session.putAttribute(outFlowFile, ERROR_CODE_ATTRIBUTE, getErrorCode(e));
		session.putAttribute(outFlowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage());

	getLogger().error("Couldn't read file metadata content as records from incoming flowfile", exception);
	getLogger().error("Failed to read input records {}", flowFile, exception);

	getLogger().error("Unexpected error while processing incoming flowfile", exception);
	getLogger().error("Failed to read input {}", flowFile, exception);

	@Tags({"samba, smb, cifs, files", "fetch"})
	@Tags({"samba", "smb", "cifs", "files", "fetch"})

	protected SmbjClientProviderService configureTestRunnerForSambaDockerContainer(TestRunner testRunner)
	protected SmbjClientProviderService configureSmbClient(TestRunner testRunner)

		testRunner.setProperty(smbjClientProviderService, DOMAIN, "domain");
		return smbjClientProviderService;

Conversation

kulikg commented Aug 9, 2022

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

exceptionfactory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turcsanyip commented Aug 29, 2022

Uh oh!

turcsanyip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!