Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
exceptionfactory closed pull request #7832: NIFI-12160: Kafka Connect: Check if all the necessary nars have been … URL: https://github.com/apache/nifi/pull/7832 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
pgyori commented on PR #7832: URL: https://github.com/apache/nifi/pull/7832#issuecomment-1759980543 Thank you @exceptionfactory ! I made the changes you recommended. Can you please recheck when your time permits? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
exceptionfactory commented on code in PR #7832:
URL: https://github.com/apache/nifi/pull/7832#discussion_r1356027290
##
nifi-external/nifi-kafka-connect/nifi-kafka-connector/src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java:
##
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.kafka.connect;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.util.Arrays;
+
+public class StatelessKafkaConnectorWorkingDirectoryUtil {
+
+public static final String NAR_UNPACKED_SUFFIX = "nar-unpacked";
+public static final String HASH_FILENAME = "nar-digest";
+private static final Logger logger =
LoggerFactory.getLogger(StatelessKafkaConnectorWorkingDirectoryUtil.class);
+
+/**
+ * Goes through the nar/extensions and extensions directories within the
working directory
+ * and deletes every directory whose name ends in "nar-unpacked" and does
not have a
+ * "nar-digest" file in it.
+ * @param workingDirectory File object pointing to the working directory.
+ */
+public static void checkWorkingDirectoryIntegrity(final File
workingDirectory) {
+purgeIncompleteUnpackedNars(new File(new File(workingDirectory,
"nar"), "extensions"));
+purgeIncompleteUnpackedNars(new File(workingDirectory, "extensions"));
+}
+
+/**
+ * Receives a directory as parameter and goes through every directory
within it that ends in
+ * "nar-unpacked". If a directory ending in "nar-unpacked" does not have a
file named
+ * "nar-digest" within it, it gets deleted with all of its contents.
+ * @param directory A File object pointing to the directory that is
supposed to contain
+ * further directories whose name ends in "nar-unpacked".
+ */
+public static void purgeIncompleteUnpackedNars(final File directory) {
+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()));
Review Comment:
The `Arrays.deepToString()` could be somewhat expensive, recommend wrapping
this log in a conditional of `isDebugEnabled()`.
##
nifi-external/nifi-kafka-connect/nifi-kafka-connector/src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorWorkingDirectoryUtil.java:
##
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.kafka.connect;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.util.Arrays;
+
+public class StatelessKafkaConnectorWorkingDirectoryUtil {
+
+public static final String NAR_UNPACKED_SUFFIX = "nar-unpacked";
+public static final String HASH_FILENAME = "nar-digest";
+private static final Logger logger =
LoggerFactory.getLogger(StatelessKafkaConnectorWorkingDirectoryUtil.class);
+
+/**
+ * Goes through the nar/extensions and extensions directories within the
working directory
+ * and deletes every directory whose name ends in "nar-unpacked" and does
not have a
+ * "nar-digest" file in it.
+ * @param workingDirectory File object pointing to
Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
pgyori commented on PR #7832: URL: https://github.com/apache/nifi/pull/7832#issuecomment-1754880810 Thank you @exceptionfactory for your feedback! I prepared the modifications you asked for. Can you please have a look at them? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
exceptionfactory commented on code in PR #7832:
URL: https://github.com/apache/nifi/pull/7832#discussion_r1344519865
##
nifi-external/nifi-kafka-connect/nifi-kafka-connector-tests/src/test/java/org/apache/nifi/kafka/connect/ITStatelessKafkaConnectorUtil.java:
##
Review Comment:
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.
##
nifi-external/nifi-kafka-connect/nifi-kafka-connector/src/main/java/org/apache/nifi/kafka/connect/StatelessKafkaConnectorUtil.java:
##
@@ -269,4 +275,53 @@ private static File detectNarDirectory() {
logger.info("Detected NAR Directory to be {}",
narDirectory.getAbsolutePath());
return narDirectory;
}
+
+private static void checkWorkingDirectoryIntegrity(final File
workingDirectory) {
+purgeIncompleteUnpackedNars(new File(new File(workingDirectory,
"nar"), "extensions"));
Review Comment:
It looks like purge method could be abstracted to a separate utility, which
would make it much easier to unit test.
##
nifi-external/nifi-kafka-connect/nifi-kafka-connector-tests/pom.xml:
##
@@ -61,10 +61,22 @@
2.0.0-SNAPSHOT
nar
+
+commons-io
+commons-io
+2.13.0
Review Comment:
This version number should be removed because it is set in the root Maven
configuration.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
pgyori commented on PR #7832: URL: https://github.com/apache/nifi/pull/7832#issuecomment-1744969962 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. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] NIFI-12160: Kafka Connect: Check if all the necessary nars have been … [nifi]
pgyori opened a new pull request, #7832: URL: https://github.com/apache/nifi/pull/7832 …fully unpacked before starting a connector # Summary [NIFI-12160](https://issues.apache.org/jira/browse/NIFI-12160) # Tracking Please complete the following tracking steps prior to pull request creation. ### Issue Tracking - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created ### Pull Request Tracking - [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-0` - [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-0` ### Pull Request Formatting - [x] Pull Request based on current revision of the `main` branch - [x] 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 - [x] Build completed using `mvn clean install -P contrib-check` - [x] JDK 21 ### Licensing - [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html) - [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files ### Documentation - [ ] Documentation formatting appears as expected in rendered files -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
