[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user b-long commented on the issue: https://github.com/apache/nifi/pull/1156 @olegz @bbende , et al. Any insight on this -> https://stackoverflow.com/questions/42938158/nifi-springcontextprocessor-using-configuration-instead-of-xml#comment84380723_42946116 ? ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 @bbende I now tend to agree. Per-lifecycle class loading is an edge case and we only use it right now in SpringContextProcessor where it is required due to the fact that Spring itself is a developer framework and it's class path heavily depends on volatile artifacts (e.g., configuration files, application code etc.). So while most JARs will remain the same, the overall classpath may change after re-starts. For the cases where one deals with JMS ConectionFactories and JDBS Drivers that most likely never be the case. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/1156 @olegz cool thanks! regarding the per-instance vs. per-lifecycle, in this first pass we are basically loading the resources at the time the properties are applied in order to be available during validation, I think in the future we can determine if we need something additional that is more tied to the component lifecycle. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 @bbende I am actually in the process of testing it now, but giving that this is class loading, give till tomorrow and if all is good I'll merge. You can squash now, I am good with the changes you've made. Also, probably lost in the maze of comments, but i don't think we ever closed the loop on per-instance vs, per-lifecycle point i've made earlier. What's your take on that? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/1156 @olegz just pushed up a commit with some changes from the last round of feedback (renaming jarFile, fixing empty catch block, fixing empty IllegalArgumentException). Have you had a chance to test this out? Also let me know if/when you want me to squash the commits --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 @bbende just left a few minor comments which probably all fall in the category of stylistic, so it's up to you to decide if you want to change anything. Now I need to spend some time and play with it to see exactly how it works. Will do it over the weekend. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 @bbende thanks, will start looking --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/1156 @olegz I pushed up a second commit that with the change that I described above, and also some of the changes from your feedback. Unfortunately I had already rebased my branch before you had started reviewing, so i ended up having to force push, but I left it as two commits where the first one has the stuff you already looked at, and the second commit has changes after that. I think the remaining discussion point is mostly around how restrictive we get regarding what happens when one or more resources can't be found. I understand the point about what happens if something that is necessary can't be loaded, but I also want to give flexibility to the developers using this feature. There may be cases where someone wants to provide a couple of directories that are common locations for a given library, and if found then load whatever is found, but if not then maybe they don't care. One option might be to provide a validator that uses ClassLoader.getURLsforClasspath to validate everything resolves, and then document that this is available, but not enforce that it is always used. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user bbende commented on the issue: https://github.com/apache/nifi/pull/1156 @olegz thanks for reviewing, I posted a couple of replies to the inline comments. I also wanted to mention that after submitting this PR, I realized that a slight modification to the approach might open this feature up to a wider variety of use cases... In the current PR, when the component is annotated with @RequiresInstanceClassLoading it creates an InstanceClassLoader which copies all of the URL resources from the NAR ClassLoader. This was necessary to solve the HBase + Phoenix problem, but it many other cases it may be acceptable to just make a new InstanceClassLoader with a parent of the NAR ClassLoader. So I started working on a change where every single processor, cs, reporting task will get an InstanceClassLoader and by default it will be empty and have a parent of the NAR ClassLoader. Only components with the @RequiresInstanceClassLoading will force a full copy of the resources. This should lets the HBaseControllerService use @RequiresInstanceClassLoading, and other components can still have properties that modify the classpath without making full copies. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 @bbende While i am still reviewing and testing, I think it is worth clarifying what per-instance really means. What I am trying to say is the fact that property values can be changed multiple times per single instance of the component and while it's ok for most properties, changing values for things like additional classpath will most likely result in unpredictable type resolution exceptions. For example; If a JAR provided as property value is changed after a component already had a chance to run and certain classes were already loaded from the previous JAR while new versions (compatible or not) of the same classes are coming with the new JAR. Alternatively, we can take a slightly different approach and address it similarly to the way it is addressed in SpringContextProcessor where additional classpath has no relation to the instance of the component and only exists while component is in active state (e.g., CS-enabled, Proc-started, etc.) To summarize; we either have to document the limitations and side-effects of the per-instance approach or change it to per-activation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi issue #1156: NIFI-2909 and NIFI-1712 Per-Instance ClassLoading
Github user olegz commented on the issue: https://github.com/apache/nifi/pull/1156 Reviewing. . . --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
