NIFI-10493 MiNiFi: Add C2 handler for Transfer/Debug operation by briansolo1985 · Pull Request #6434 · apache/nifi

briansolo1985 · 2022-09-20T18:56:51Z

Summary

NIFI-10493

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

ferencerdei

Thanks for the contribution, I have some recommendations. Please have a look.

ferencerdei · 2022-09-21T13:32:10Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+            try {
+                Path tempDirectory = createTempDirectory(null);
+                Path tempFile = Paths.get(tempDirectory.toAbsolutePath().toString(), fileName);
+                write(tempFile, (Iterable<String>) lines(path).filter(contentFilter)::iterator);


I think lines() needs to be used with try with resource

Thanks for spotting this, I missed that

ferencerdei · 2022-09-21T13:33:12Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+    }
+
+    public Optional<byte[]> createDebugBundle(List<Path> filePaths) {
+        if (filePaths.size() == 0) {


You can use the isEmpty() method

ferencerdei · 2022-09-21T13:36:43Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+            }
+        } catch (Exception e) {
+            LOG.error("Error during create compressed bundle", e);
+            closeQuietly(byteOutputStream);


can't we use try with resources here also?
Also It's not clear why do we need to call the close here and in the finally as well.

Unfortunately we can't. I need the ByteArrayOutputStream reference to return with byte[] in the end. However if I return with byteOutputStream.toByteArray() from the try-with-resources block, it will return with a malformed result. It seems byteOutputStream needs to be closed before it can be converted to a byte array.
Regarding the two closeQuietly, I had to close the stream in the catch block before nulling the reference out, so it couldn't be closed in the finally clause. I changed this part to be more straightforward.

ferencerdei · 2022-09-21T13:41:31Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+    }
+
+    enum LogFile implements DebugBundleFile {
+        APP_LOG_FILE("minifi-app.log"),


The filenames depend on the logback configuration. It's not ideal to hardcode this. Can we use it from a property?

Thanks for highlighting this. Now these are injected via properties

ferencerdei · 2022-09-21T13:46:52Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+    }
+
+    private void cleanup(List<Path> paths) {
+        paths.forEach(path -> {


what do you think about removing the temporary directory as well?

Added logic to remove the temporary directory

ferencerdei · 2022-09-21T13:50:46Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+    }
+
+    enum ConfigFile implements DebugBundleFile {
+        CONFIG_YML_FILE("config.yml"),


These file names are also configurable via environment and bootsrap.conf variables, therefore we shouldn't hardcode.

Fixed this as well

bejancsaba

Thank you for this new exciting improvement. I agree with the comments from @ferencerdei on top of those I added only a few. The big one would be the one around file names. If these are addressed I think we are good to merge.

bejancsaba · 2022-09-22T11:58:14Z

...-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java

+                .build())
+            .build();
+
+        logger.info("Uploading debug bundle to url={} size={}", debugCallbackUrl, debugBundle.length);


Super minor just for the sake of consistency sometimes url is logged just as part of the text, sometimes is in brackets [] and sometimes is behind "=". I'm pretty sure it is inconsistent outside this change as well just I saw these 3 patterns in this change. I'm ok with whatever approach so will leave that to you.

Standardized the log messages

bejancsaba · 2022-09-22T12:09:43Z

c2/c2-client-bundle/c2-client-service/pom.xml

+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-compress</artifactId>
+            <version>1.21</version>


Please externalise the version to the main nifi pom so everything is in one place.

I checked the occurrences of this dependency in other modules. Everywhere it is used with version. To not break consistency I would leave this as it is, if you don't mind

bejancsaba · 2022-09-22T12:09:55Z

c2/c2-client-bundle/c2-client-api/src/main/java/org/apache/nifi/c2/client/api/C2Client.java


    /**
-     * Retrive the content of the new flow from the C2 Server
+     * Retrieve the content of the new flow from the C2 Server


bejancsaba · 2022-09-22T12:29:23Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+
+    @Override
+    public C2OperationAck handle(C2Operation operation) {
+        Function<C2OperationState, C2OperationAck> operationAckCreate = operationAck(operation);


Is there a specific need for this function based approach? Extending the function to get 2 parameters looks to be sufficient and simpler. What do you think? Or maybe there is something that I'm missing?

Tried to use a more functional approach, but agree, Java syntax is a bit cluncky for this. Refactored to a 2 parameters accepting method

ferencerdei · 2022-09-23T08:26:16Z

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

+
+    enum ConfigFile implements DebugBundleFile {
+        CONFIG_YML_FILE("config.yml"),
+        CONFIG_NEW_YML_FILE("config-new.yml"),


I think we don't need to add the config-new.yml to the debug bundle. It's basically a temporary file that is used to construct the config.yml during config updates.

Thanks, excluded it from the list of the files

# Conflicts: # c2/c2-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java # nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/c2/C2NifiClientService.java

bejancsaba

Conceptually it looks really good so I had just minor lower level comments regarding structure and naming should be really simple to address. After the NiFi release we should be good to merge.

...-client-bundle/c2-client-http/src/main/java/org/apache/nifi/c2/client/http/C2HttpClient.java

...service/src/main/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandler.java

...ice/src/test/java/org/apache/nifi/c2/client/service/operation/DebugOperationHandlerTest.java

bejancsaba

+1 from my side all the changes look good, thanks for applying those

This closes apache#6434 Signed-off-by: Ferenc Erdei <erdei.ferenc90@gmail.com>

NIFI-10458 MiNiFi: Add C2 handler for Transfer/Debug operation

8bcad1a

briansolo1985 force-pushed the NIFI-10458_C2_DEBUG branch from 0aedc71 to 8bcad1a Compare September 20, 2022 18:58

ferencerdei added the minifi Pull requests that updates minifi/c2 codes label Sep 21, 2022

ferencerdei requested changes Sep 21, 2022

View reviewed changes

briansolo1985 changed the title ~~NIFI-10458 MiNiFi: Add C2 handler for Transfer/Debug operation~~ NIFI-10493 MiNiFi: Add C2 handler for Transfer/Debug operation Sep 21, 2022

bejancsaba requested changes Sep 22, 2022

View reviewed changes

ferencerdei reviewed Sep 23, 2022

View reviewed changes

briansolo1985 added 2 commits September 24, 2022 12:10

NIFI-10458 Incorporating review comments

e1152aa

briansolo1985 requested review from bejancsaba and ferencerdei and removed request for ferencerdei September 24, 2022 12:23

NIFI-10458 Fix unit test failure on Windows

da10d5f

bejancsaba requested changes Sep 30, 2022

View reviewed changes

NIFI-10458 Additional review comment fixes

6a07ddf

bejancsaba approved these changes Sep 30, 2022

View reviewed changes

ferencerdei approved these changes Oct 4, 2022

View reviewed changes

ferencerdei closed this in 16bcb8f Oct 4, 2022

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

NIFI-10458 MiNiFi: Add C2 handler for Transfer/Debug operation

926a493

This closes apache#6434 Signed-off-by: Ferenc Erdei <erdei.ferenc90@gmail.com>

Conversation

briansolo1985 commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

Uh oh!

ferencerdei left a comment

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!

briansolo1985 Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bejancsaba left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

bejancsaba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

briansolo1985 commented Sep 20, 2022 •

edited

Loading

briansolo1985 Sep 21, 2022 •

edited

Loading