[ 
https://issues.apache.org/jira/browse/NIFI-2909?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15630128#comment-15630128
 ] 

ASF GitHub Bot commented on NIFI-2909:
--------------------------------------

Github user olegz commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/1156#discussion_r86222278
  
    --- Diff: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core-api/src/main/java/org/apache/nifi/controller/AbstractConfiguredComponent.java
 ---
    @@ -141,48 +191,64 @@ public void setProperty(final String name, final 
String value) {
          * @return true if removed; false otherwise
          * @throws java.lang.IllegalArgumentException if the name is null
          */
    -    @Override
    -    public boolean removeProperty(final String name) {
    +    private boolean removeProperty(final String name) {
             if (null == name) {
                 throw new IllegalArgumentException();
             }
     
    -        lock.lock();
    -        try {
    -            verifyModifiable();
    -
    -            try (final NarCloseable narCloseable = 
NarCloseable.withComponentNarLoader(component.getClass())) {
    -                final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
    -                String value = null;
    -                if (!descriptor.isRequired() && (value = 
properties.remove(descriptor)) != null) {
    -
    -                    if (descriptor.getControllerServiceDefinition() != 
null) {
    -                        if (value != null) {
    -                            final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(value);
    -                            if (oldNode != null) {
    -                                oldNode.removeReference(this);
    -                            }
    -                        }
    -                    }
    +        final PropertyDescriptor descriptor = 
component.getPropertyDescriptor(name);
    +        String value = null;
    +        if (!descriptor.isRequired() && (value = 
properties.remove(descriptor)) != null) {
     
    -                    try {
    -                        component.onPropertyModified(descriptor, value, 
null);
    -                    } catch (final Exception e) {
    -                        // nothing really to do here...
    +            if (descriptor.getControllerServiceDefinition() != null) {
    +                if (value != null) {
    +                    final ControllerServiceNode oldNode = 
serviceProvider.getControllerServiceNode(value);
    +                    if (oldNode != null) {
    +                        oldNode.removeReference(this);
                         }
    -
    -                    return true;
                     }
                 }
    -        } finally {
    -            lock.unlock();
    +
    +            try {
    +                component.onPropertyModified(descriptor, value, null);
    +            } catch (final Exception e) {
    +                // nothing really to do here...
    +            }
    +
    +            return true;
             }
    +
             return false;
         }
     
    +    /**
    +     * Adds all of the modules identified by the given module paths to the 
InstanceClassLoader for this component.
    +     *
    +     * @param modulePaths a list of module paths where each entry can be a 
comma-separated list of multiple module paths
    +     */
    +    private void processClasspathModifiers(final Set<String> modulePaths) {
    +        try {
    +            final URL[] urls = 
ClassLoaderUtils.getURLsForClasspath(modulePaths, null, true);
    +
    +            final ClassLoader classLoader = 
Thread.currentThread().getContextClassLoader();
    +            if (!(classLoader instanceof InstanceClassLoader)) {
    +                // Really shouldn't happen, but if we somehow got here and 
don't have an InstanceClassLoader then log a warning and move on
    +                logger.warn("Unable to modify the classpath for {}, 
expected InstanceClassLoader, but found {}",
    +                        new Object[] { name, 
classLoader.getClass().getName()});
    +                return;
    --- End diff --
    
    Agree with the first scenario.
    As for the ```ClassLoaderUtils.getURLsForClasspath(...)```, I've read what 
you describe and I personally lean to the side of fail fast. Don't get me 
wrong; what you describe is perfectly valid, but I approach it very differently 
where the _fatal_ condition I am referring to will most definitely be a flow 
developer's error which will be caught during the development of the flow and 
therefore is not expected to happen in prod, so delegating to the component 
developer to do custom validation in each and every new/existing component 
seems like on overhead.


> Provide a framework mechanism for loading additional classpath resources
> ------------------------------------------------------------------------
>
>                 Key: NIFI-2909
>                 URL: https://issues.apache.org/jira/browse/NIFI-2909
>             Project: Apache NiFi
>          Issue Type: Improvement
>            Reporter: Bryan Bende
>            Assignee: Bryan Bende
>             Fix For: 1.1.0
>
>
> We currently have several components with a property for specifying 
> additional classpath resources (DBCP connection pool, scripting processors, 
> JMS). Each of these components is responsible for handling this in its own 
> way. 
> The framework should provide a more integrated solution to make it easier for 
> component developers to deal with this scenario. Some requirements that need 
> to be met by this solution:
> - Multiple instances of the same component with different resources added to 
> the classpath and not interfering with each other (i.e. two DBCP connection 
> pools using different drivers)
> - Ability to modify the actual ClassLoader of the component to deal with 
> frameworks that use Class.forName() without passing in a ClassLoader, meaning 
> if a processor loads class A and class A calls Class.forName(classBName), 
> then class B needs to be available in the ClassLoader that loaded the 
> processor's class which in turn loaded class A
> - A component developer should be able to indicate that a given 
> PropertyDescriptor represents a classpath resource and the framework should 
> take care of the ClassLoader manipulation



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to