You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by bbende <gi...@git.apache.org> on 2018/05/01 16:11:04 UTC
[GitHub] nifi pull request #2668: NIFI-5136 Ensure processor references are removed f...
GitHub user bbende opened a pull request:
https://github.com/apache/nifi/pull/2668
NIFI-5136 Ensure processor references are removed from LogRepository …
…and from ProcessScheduler
- Forcing FileSystem statistics thread to be interrupted when HDFS processors are stopped
- Stop creating temp components during import from registry, use bundle info instead
Thank you for submitting a contribution to Apache NiFi.
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 master)?
- [ ] Is your initial contribution a single, squashed commit?
### 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?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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 travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bbende/nifi stop-leaking-processors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2668.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2668
----
commit 0e5b55527cd9c957f4b8015d799c526012b7c883
Author: Bryan Bende <bb...@...>
Date: 2018-04-27T18:52:58Z
NIFI-5136 Ensure processor references are removed from LogRepository and from ProcessScheduler
- Forcing FileSystem statistics thread to be interrupted when HDFS processors are stopped
- Stop creating temp components during import from registry, use bundle info instead
----
---
[GitHub] nifi issue #2668: NIFI-5136 Ensure processor references are removed from Log...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2668
@bbende thanks for addressing these issues! I can see that we've tackled a handful of spots that could be leaking the references, and I've seen the heap dumps showing that they are no longer concerns. The changes to the Abstract Hadoop Processors are less desirable than I'd prefer, but I agree that the API doesn't really expose what it would need to expose in order to do this more effectively, so it's a reasonable workaround. The LogRepositoryFactory change in FlowController's create* methods is good - i hadn't realized that we were doing that there, within the nar loader.
With this particular scenario, I don't think it's really replicable in a unit test. All that could be done in unit tests would be to verify that very trivial functionality like "removeLogger() calls remove() on the underlying map" which make for bad unit testing IMO. You could attempt to create some very involved integration tests, but they would involve mocking out so much that it would be hard to know what is really being verified. On top of that, you still would not be able to verify that the appropriate objects are reclaimable via garbage collection.
This all looks good to me, though. Can verify behavior and code changes look good. +1 will merge to master.
---
[GitHub] nifi issue #2668: NIFI-5136 Ensure processor references are removed from Log...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi/pull/2668
@ottobackwards I identified the leaks using VisualVM to analyze the heap, and can now see that the processors and class loader references are no longer in the heap after these changes
---
[GitHub] nifi pull request #2668: NIFI-5136 Ensure processor references are removed f...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2668#discussion_r185276958
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-hadoop-utils/src/main/java/org/apache/nifi/processors/hadoop/AbstractHadoopProcessor.java ---
@@ -257,9 +260,63 @@ public final void abstractOnScheduled(ProcessContext context) throws IOException
@OnStopped
public final void abstractOnStopped() {
+ final HdfsResources resources = hdfsResources.get();
+ if (resources != null) {
+ // Attempt to close the FileSystem
+ final FileSystem fileSystem = resources.getFileSystem();
+ try {
+ interruptStatisticsThread(fileSystem);
+ } catch (Exception e) {
+ getLogger().warn("Error stopping FileSystem statistics thread: " + e.getMessage(), e);
+ } finally {
+ if (fileSystem != null) {
+ try {
+ fileSystem.close();
+ } catch (IOException e) {
+ getLogger().warn("Error close FileSystem: " + e.getMessage(), e);
+ }
+ }
+ }
+
+ // Clean-up the static reference to the Configuration instance
+ UserGroupInformation.setConfiguration(new Configuration());
+
+ // Clean-up the reference to the InstanceClassLoader that was put into Configuration
+ final Configuration configuration = resources.getConfiguration();
+ configuration.setClassLoader(null);
+
+ // Need to remove the Provider instance from the JVM's Providers class so that InstanceClassLoader can be GC'd eventually
+ final SaslPlainServer.SecurityProvider saslProvider = new SaslPlainServer.SecurityProvider();
+ Security.removeProvider(saslProvider.getName());
+ }
+
+ // Clear out the reference to the resources
hdfsResources.set(new HdfsResources(null, null, null));
}
--- End diff --
You *could* use commons reflect here, but I'm not sure it matters
---
[GitHub] nifi pull request #2668: NIFI-5136 Ensure processor references are removed f...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2668#discussion_r185277171
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-hadoop-utils/src/main/java/org/apache/nifi/processors/hadoop/AbstractHadoopProcessor.java ---
@@ -257,9 +260,63 @@ public final void abstractOnScheduled(ProcessContext context) throws IOException
@OnStopped
public final void abstractOnStopped() {
--- End diff --
Is there anything common to other processors here? Are we going to end up pointing other processors with this problem, one after another to this code?
---
[GitHub] nifi issue #2668: NIFI-5136 Ensure processor references are removed from Log...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/nifi/pull/2668
Is there a test case with the original ML user's issue that can have a test done? What would also be nice if these things that you changed had a 'report' method, kind of like the Nar utils stuff has for extensions etc..
---
[GitHub] nifi pull request #2668: NIFI-5136 Ensure processor references are removed f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2668
---