NIFI-8380: Allow for an extensions.directory property to specify wher… by markap14 · Pull Request #4950 · apache/nifi

markap14 · 2021-03-29T20:56:48Z

…e to place downloaded files. Also fixed an issue that was encountered, when a Source Processor is scheduled for Primary Node Only but more than 1 task is set. In that case, even though only a single task will should be scheduled, an Exception was getting thrown because @OnScheduled methods of Processors were still called. To avoid this, moved the initialization of the dataflow outside of the creation of the dataflow so that initialization can be triggered only when appropriate.

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

…e to place downloaded files. Also fixed an issue that was encountered, when a Source Processor is scheduled for Primary Node Only but more than 1 task is set. In that case, even though only a single task will should be scheduled, an Exception was getting thrown because @OnScheduled methods of Processors were still called. To avoid this, moved the initialization of the dataflow outside of the creation of the dataflow so that initialization can be triggered only when appropriate.

akatona84 · 2021-03-30T07:14:21Z

.../src/main/java/org/apache/nifi/kafka/connect/validators/ConnectDirectoryExistsValidator.java


        final File file = new File((String) value);
-        if (!file.exists()) {
+        if (!file.exists() && !file.mkdirs()) {


Is this necessary to create the dir in the validation phase?

No, you're right, that shouldn't be there. I put it there as a bit of an experiment, and forgot to remove it.

akatona84 · 2021-03-30T07:18:49Z

...undle/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/flow/StatelessDataflow.java

@@ -24,8 +24,33 @@
 import java.util.Set;

 public interface StatelessDataflow {


It is not part of this PR, but wouldn't make sense to make it extend AutoClosable? and having close() instead of shutdown(), etc.

I don't think we should extend AutoCloseable in this case. The StatelessDataflow is not really a "resource" and is generally not used in a such a way that you'd want to use a try-with-resources to create it for a short period of time and then cleanup. Rather, shutdown() is generally used when the application is shutdown. This involves terminating thread pools, cleaning up potentially many resources, etc., etc.

...undle/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/flow/StatelessDataflow.java

…d extensions directory to exist; removed auto-creation of directories in validation

urbandan

I think the download related changes should be mentioned in the commit message.

urbandan · 2021-04-01T06:50:48Z

.../src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java


+        final String extensionsDirectoryFilename = properties.getProperty(EXTENSIONS_DIRECTORY);
+        final File extensionsDirectory = extensionsDirectoryFilename == null ? narDirectory : new File(extensionsDirectoryFilename);
+        if (!extensionsDirectory.exists() && !extensionsDirectory.mkdirs()) {


I'm not sure if the mkdirs call is ok here: the validator will fail if the dir doesn't exist, but the actual connector/task init will pass if the dir can be created. Seems like the validator is stricter than it should be.

The validation does not fail if the directory doesn't exist - that validation was removed.

Got it, thanks

…eads trying to download/unpack extensions, we properly synchronize the unpacking and unpack into the correct sub-directory under the working directory

urbandan · 2021-04-06T06:22:34Z

...tateless-bootstrap/src/main/java/org/apache/nifi/stateless/bootstrap/StatelessBootstrap.java

        final long unpackStart = System.currentTimeMillis();
        final Predicate<BundleCoordinate> narFilter = coordinate -> true;
-        NarUnpacker.unpackNars(systemBundle, frameworkWorkingDir, extensionsWorkingDir, null, narDirectories, false, false, false, narFilter);
+        NarUnpackLock.lock();


Is this lock also protecting the part when the nars are getting downloaded? Can it cause issues if the download is not protected by it?
With this solution, do we avoid downloading the nars multiple times? (i.e. if one task is blocked on getting the lock, will it realize that the sibling task already downloaded and unpacked the necessary nars?)

No, each process that needs the nar will download it for itself. They download using a temporary filename. Only one task will manage to rename its download to the 'final' name. Others will delete their temporary files. It's not efficient, but it was a simple/straight-forward solution that can be improved upon later.

urbandan · 2021-04-06T06:23:01Z

.../src/main/java/org/apache/nifi/stateless/config/PropertiesFileEngineConfigurationParser.java


+        final String extensionsDirectoryFilename = properties.getProperty(EXTENSIONS_DIRECTORY);
+        final File extensionsDirectory = extensionsDirectoryFilename == null ? narDirectory : new File(extensionsDirectoryFilename);
+        if (!extensionsDirectory.exists() && !extensionsDirectory.mkdirs()) {


Got it, thanks

urbandan · 2021-04-06T06:24:24Z

...-bundle/nifi-stateless-api/src/main/java/org/apache/nifi/stateless/engine/NarUnpackLock.java

+ * (due to multiple classloders being used for the 'stateless nar'), we define a singleton Lock within the nifi-stateless-api module.
+ * This lock should always be obtained before attempting to unpack nars.
+ */
+public class NarUnpackLock {


As an improvement, this could be providing locks on a per-directory basis - so if we have multiple NiFi Connectors with separate nar directories, they can progress without blocking each other.

Yes, we can certainly improve this in the future. Given that this happens only at startup and likely will only take hundreds of milliseconds, I'm not particularly worried about the performance impact initially. But we can certainly improve this process if we need to, going forward.

akatona84

Tested a bit on a cluster, seems to be working now!

pvillard31 · 2021-04-07T11:28:45Z

Merged to main, thanks @markap14 @akatona84 @urbandan

markap14 · 2021-04-07T12:23:36Z

Awesome! Thanks @akatona84 and @urbandan for such thorough testing & review! And thanks @pvillard31 for reviewing & merging!

…e to place downloaded files. Also fixed an issue that was encountered, when a Source Processor is scheduled for Primary Node Only but more than 1 task is set. In that case, even though only a single task will should be scheduled, an Exception was getting thrown because @OnScheduled methods of Processors were still called. To avoid this, moved the initialization of the dataflow outside of the creation of the dataflow so that initialization can be triggered only when appropriate. NIFI-8380: Removed requirement in validation for working directory and extensions directory to exist; removed auto-creation of directories in validation NIFI-8380: Fixed a few thrading bugs, so that if we have multiple threads trying to download/unpack extensions, we properly synchronize the unpacking and unpack into the correct sub-directory under the working directory Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#4950.

akatona84 reviewed Mar 30, 2021

View reviewed changes

NIFI-8380: Removed requirement in validation for working directory an…

2fcbc52

…d extensions directory to exist; removed auto-creation of directories in validation

urbandan reviewed Apr 1, 2021

View reviewed changes

NIFI-8380: Fixed a few thrading bugs, so that if we have multiple thr…

2097736

…eads trying to download/unpack extensions, we properly synchronize the unpacking and unpack into the correct sub-directory under the working directory

urbandan reviewed Apr 6, 2021

View reviewed changes

akatona84 approved these changes Apr 6, 2021

View reviewed changes

urbandan approved these changes Apr 7, 2021

View reviewed changes

asfgit closed this in b799879 Apr 7, 2021

pvillard31 approved these changes Apr 7, 2021

View reviewed changes

		@@ -24,8 +24,33 @@
		import java.util.Set;

		public interface StatelessDataflow {

Conversation

markap14 commented Mar 29, 2021

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

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!

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

akatona84 left a comment

Choose a reason for hiding this comment

Uh oh!

pvillard31 commented Apr 7, 2021

Uh oh!

markap14 commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants