Skip to content

NIFI-12160: Kafka Connect: Check if all the necessary nars have been …#7832

Closed
pgyori wants to merge 4 commits intoapache:mainfrom
pgyori:NIFI-12160
Closed

NIFI-12160: Kafka Connect: Check if all the necessary nars have been …#7832
pgyori wants to merge 4 commits intoapache:mainfrom
pgyori:NIFI-12160

Conversation

@pgyori
Copy link
Contributor

@pgyori pgyori commented Oct 3, 2023

…fully unpacked before starting a connector

Summary

NIFI-12160

Tracking

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

Issue Tracking

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 21

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

@pgyori
Copy link
Contributor Author

pgyori commented Oct 3, 2023

The integration test ITStatelessKafkaConnectorUtil.testCreateDataflowThenCorruptWorkingDirectoryAndRedeployDataflow() currently can only be executed on NiFi 1.x branch because it needs to fetch the most recent version of the Stateless NiFi Kafka Connector Plugin from the repository https://repository.apache.org/content/repositories/releases/org/apache/nifi/nifi-kafka-connector-assembly/ and the most recent build of the plugin in the repo (v.1.23.2) is not compatible with the changes in NiFi 2.x.

exceptionfactory
exceptionfactory requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on improving the behavior @pgyori. As noted in the detailed comments, I am concerned about introducing a new integration test for this specific issue. It looks like the purge logic could be abstracted out, which would make it much easier to unit test quickly. What do you think of that approach?

nifi-external/nifi-kafka-connect/nifi-kafka-connector-tests/pom.xml Outdated
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.13.0</version>
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version number should be removed because it is set in the root Maven configuration.

...nnector-tests/src/test/java/org/apache/nifi/kafka/connect/ITStatelessKafkaConnectorUtil.java Outdated
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this exercise the issue, we should avoid adding new integration tests unless they provide significant value. Integration tests outside of nifi-system-test-suite are not executed in automated workflows and canmore stale when refactoring.

...kafka-connector/src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorUtil.java Outdated
}

private static void checkWorkingDirectoryIntegrity(final File workingDirectory) {
purgeIncompleteUnpackedNars(new File(new File(workingDirectory, "nar"), "extensions"));
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like purge method could be abstracted to a separate utility, which would make it much easier to unit test.

@pgyori
Copy link
Contributor Author

pgyori commented Oct 10, 2023

Thank you @exceptionfactory for your feedback!
I prepared the modifications you asked for. Can you please have a look at them?

pgyori added 2 commits October 10, 2023 12:17
@pgyori
Revert "NIFI-12160: Kafka Connect: Check if all the necessary nars ha…
4ac9972 
…ve been fully unpacked before starting a connector"

This reverts commit 4337982.
@pgyori
NIFI-12160: Kafka Connect: Check if all the necessary nars have been …
187a40b 
…fully unpacked before starting a connector
exceptionfactory
exceptionfactory requested changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the adjustments and refactoring the testing approach @pgyori, the updated version appears more straightforward. I noted several naming adjustment recommendations, but this looks closer to completion.

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
final File[] unpackedDirs = directory.listFiles(file -> file.isDirectory() && file.getName().endsWith(NAR_UNPACKED_SUFFIX));
if (unpackedDirs == null || unpackedDirs.length == 0) {
logger.debug("Found no unpacked NARs in {}", directory);
logger.debug("Directory contains: {}", Arrays.deepToString(directory.listFiles()));
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Arrays.deepToString() could be somewhat expensive, recommend wrapping this log in a conditional of isDebugEnabled().

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
Comment on lines +60 to +64
if (!narHashFile.exists()) {
purgeDirectory(unpackedDir);
} else {
logger.debug("Already successfully unpacked {}", unpackedDir);
}
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend reversing the logic to make it more straightforward:

Suggested change
if (!narHashFile.exists()) {
purgeDirectory(unpackedDir);
} else {
logger.debug("Already successfully unpacked {}", unpackedDir);
}
if (narHashFile.exists()) {
logger.debug("Already successfully unpacked {}", unpackedDir);
} else {
purgeDirectory(unpackedDir);
}

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
deleteOrDebug(fileOrDirectory);
}

private static void deleteOrDebug(final File file) {
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend renaming to deleteQuietly.

Suggested change
private static void deleteOrDebug(final File file) {
private static void deleteQuietly(final File file) {

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
* "nar-digest" file in it.
* @param workingDirectory File object pointing to the working directory.
*/
public static void checkWorkingDirectoryIntegrity(final File workingDirectory) {
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation comment is helpful, but the name of signature of the method are not very clear. What do you think of naming this something like reconcileWorkingDirectory?

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
Comment on lines +28 to +29
public static final String NAR_UNPACKED_SUFFIX = "nar-unpacked";
public static final String HASH_FILENAME = "nar-digest";
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be marked as protected since the only outside reference appears to be the test class?

...src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java Outdated
import java.io.File;
import java.util.Arrays;

public class StatelessKafkaConnectorWorkingDirectoryUtil {
Copy link
Contributor

@exceptionfactory exceptionfactory Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this class is already in the kafka.connect package, the name can be simplified. The general convention for utilities is ending with Utils. What do you think about WorkingDirectoryUtils?

@pgyori
Copy link
Contributor Author

pgyori commented Oct 12, 2023

Thank you @exceptionfactory ! I made the changes you recommended. Can you please recheck when your time permits?

exceptionfactory
exceptionfactory approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the adjustments @pgyori! +1 merging

exceptionfactory pushed a commit that referenced this pull request Oct 13, 2023
@pgyori @exceptionfactory
NIFI-12160 Kafka Connect: Check for NAR unpacking before starting
19153a7 
Check that required NAR files are unpacked completely before starting the Kafka Connector

This closes #7832

Signed-off-by: David Handermann <exceptionfactory@apache.org>
(cherry picked from commit b2e3898)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@exceptionfactory exceptionfactory exceptionfactory approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pgyori @exceptionfactory