Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      The idea of having separate classloaders for each component may be counter productive. This aims to add a jar dependency to the collection itself and each core belonging to that collection will have the jar in the classpath

      As we load everything from the .system collection , we cannot make the core loading delayed till .system is fully loaded and is available .

      There is a new set of config commands to manage the dependencies on a collection level. It is possible to have more than one jar as a dependency. The "lib" attribute is same as the blob name that we use in the blob store API
      example:

      curl http://localhost:8983/solr/collection1/config -H 'Content-type:application/json'  -d '{
      "add-runtimelib" : {"name": "jarname" , "version":2 },
      "update-runtimelib" :{"name": "jarname" ,"version":3},
      "delete-runtimelib" :"jarname" 
      }' 
      

      The same is added to the overlay.json .

      The default SolrResourceLoader does not have visibility to these jars . There is a classloader that can access these jars which is made available only to those components which are specially annotated

      Every pluggable component can have an optional extra attribute called runtimeLib=true, which means, these components are not be loaded at core load time. They are tried to be loaded on demand and if all the dependency jars are not available at the component load time an error is thrown .

      example of registering a valueSourceParser which depends on the runtime classloader

      curl http://localhost:8983/solr/collection1/config -H 'Content-type:application/json'  -d '{
      "create-valuesourceparser" : {"name": "nvl" ,
      "runtimeLib": true, 
      "class":"solr.org.apache.solr.search.function.NvlValueSourceParser , 
      "nvlFloatValue":0.0 }  
      }'
      
      1. SOLR-7073_5x.patch
        142 kB
        Noble Paul
      2. SOLR-7073.patch
        138 kB
        Noble Paul
      3. SOLR-7073.patch
        130 kB
        Noble Paul
      4. SOLR-7073.patch
        128 kB
        Noble Paul
      5. SOLR-7073.patch
        116 kB
        Noble Paul

        Issue Links

          Activity

          Hide
          Noble Paul added a comment -

          first cut

          Show
          Noble Paul added a comment - first cut
          Hide
          Noble Paul added a comment - - edited

          This does a complete refactor of Solr's plugin loading because all components now need to be lazily loadable based on the extra flag. I would love to have extra eyes on this before it is too late.

          There is a new class called PluginRegistry which is slightly more than our Map<String,Plugin>> .PluginRegistry takes care of loading the plugins at startup or lazily depending on the various attributes such as startup="lazy" or runtimeLib="true" . This patch gets rid of the Lazy loading wrappers we had for SolrRequestHandler and QueryResponseWriter

          Show
          Noble Paul added a comment - - edited This does a complete refactor of Solr's plugin loading because all components now need to be lazily loadable based on the extra flag. I would love to have extra eyes on this before it is too late. There is a new class called PluginRegistry which is slightly more than our Map<String,Plugin>> . PluginRegistry takes care of loading the plugins at startup or lazily depending on the various attributes such as startup="lazy" or runtimeLib="true" . This patch gets rid of the Lazy loading wrappers we had for SolrRequestHandler and QueryResponseWriter
          Hide
          Noble Paul added a comment -

          with basic tests added and some bugs fixed

          Show
          Noble Paul added a comment - with basic tests added and some bugs fixed
          Hide
          Noble Paul added a comment -

          more tests and cleanup

          Show
          Noble Paul added a comment - more tests and cleanup
          Hide
          Shalin Shekhar Mangar added a comment -

          Thanks Noble. Some comments:

          1. I don't like that delete-runtimelib has just the name and not a json body. In future if we want to delete old versions of jars, how do we do that? Would it delete all versions? We should keep it consistent and have a json object as the value.
          2. Minor typos in JarRepository - s/decerease/decrease and s/getSytemCollReplica/getSystemCollReplica.
          3. We should check for liveness as well inside getSystemCollReplica
          4. RecoveryStrategy has some code commented out. Is it not necessary anymore?
            /*if (handler instanceof LazyRequestHandlerWrapper) {
                  handler = ((LazyRequestHandlerWrapper) handler).getWrappedHandler();
                }*/
            
          5. The patch removes the disableExternalLib flag from RequestHandlers. Is that not needed anymore?
          6. Can we please rename SolrConfig.clsVsInfo to classVsInfo? and LazyRHHolder to LazyRequestHandlerHolder?
          7. Why is LazyRHHolder required? There are other plugins which can be loaded from runtimeLib then what is special about request handlers to deserve their own class. Also LazyRHHolder asserts that the "lib" tag must also have "version" but other lazy plugins don't?
          8. A request handler loaded from runtimeLib is initialized from LazyRHHolder but a lazy handler loaded from regular jars is loaded via LazyPluginHolder? This is really confusing. Please consider better names for these class so that their purpose is clear. Some javadocs/comments won't hurt.
          9. RequestHandlers.initHandlersFromConfig used to call init on the handlers. I can't find where they are being initialized now.
          Show
          Shalin Shekhar Mangar added a comment - Thanks Noble. Some comments: I don't like that delete-runtimelib has just the name and not a json body. In future if we want to delete old versions of jars, how do we do that? Would it delete all versions? We should keep it consistent and have a json object as the value. Minor typos in JarRepository - s/decerease/decrease and s/getSytemCollReplica/getSystemCollReplica. We should check for liveness as well inside getSystemCollReplica RecoveryStrategy has some code commented out. Is it not necessary anymore? /* if (handler instanceof LazyRequestHandlerWrapper) { handler = ((LazyRequestHandlerWrapper) handler).getWrappedHandler(); }*/ The patch removes the disableExternalLib flag from RequestHandlers. Is that not needed anymore? Can we please rename SolrConfig.clsVsInfo to classVsInfo? and LazyRHHolder to LazyRequestHandlerHolder? Why is LazyRHHolder required? There are other plugins which can be loaded from runtimeLib then what is special about request handlers to deserve their own class. Also LazyRHHolder asserts that the "lib" tag must also have "version" but other lazy plugins don't? A request handler loaded from runtimeLib is initialized from LazyRHHolder but a lazy handler loaded from regular jars is loaded via LazyPluginHolder? This is really confusing. Please consider better names for these class so that their purpose is clear. Some javadocs/comments won't hurt. RequestHandlers.initHandlersFromConfig used to call init on the handlers. I can't find where they are being initialized now.
          Hide
          Noble Paul added a comment -

          Thanks for the comments

          I don't like that delete-runtimelib has just the name and not a json body. In future if we want to delete old versions of jars,

          There is one an only one version of a given jar that you can add to a collection. So, you can either delete or update

          Minor typos in JarRepository - s/decerease/decrease and s/getSytemCollReplica/getSystemCollReplica.

          sure

          We should check for liveness as well inside getSystemCollReplica

          sure

          RecoveryStrategy has some code commented out. Is it not necessary anymore?

          We no more have those Lazy Wrappers . What we have is LazyPluginHolder . When you do a get() for a plugin, you get a live instance. fully instantiated

          The patch removes the disableExternalLib flag from RequestHandlers. Is that not needed anymore?

          That is moved to PluginRegistry where it assumes a new name enable.runtime.lib

          Can we please rename SolrConfig.clsVsInfo to classVsInfo? and LazyRHHolder to LazyRequestHandlerHolder?

          sure

          Why is LazyRHHolder required?

          It was required because of the feature we released in 5.0 for a per request handler classloader. I'm going to remove that

          RequestHandlers.initHandlersFromConfig used to call init on the handlers. I can't find where they are being initialized now.

          It all goes into PluginsRegistry . Initialization is done inside there

          Show
          Noble Paul added a comment - Thanks for the comments I don't like that delete-runtimelib has just the name and not a json body. In future if we want to delete old versions of jars, There is one an only one version of a given jar that you can add to a collection. So, you can either delete or update Minor typos in JarRepository - s/decerease/decrease and s/getSytemCollReplica/getSystemCollReplica. sure We should check for liveness as well inside getSystemCollReplica sure RecoveryStrategy has some code commented out. Is it not necessary anymore? We no more have those Lazy Wrappers . What we have is LazyPluginHolder . When you do a get() for a plugin, you get a live instance. fully instantiated The patch removes the disableExternalLib flag from RequestHandlers. Is that not needed anymore? That is moved to PluginRegistry where it assumes a new name enable.runtime.lib Can we please rename SolrConfig.clsVsInfo to classVsInfo? and LazyRHHolder to LazyRequestHandlerHolder? sure Why is LazyRHHolder required? It was required because of the feature we released in 5.0 for a per request handler classloader. I'm going to remove that RequestHandlers.initHandlersFromConfig used to call init on the handlers. I can't find where they are being initialized now. It all goes into PluginsRegistry . Initialization is done inside there
          Hide
          ASF subversion and git services added a comment -

          Commit 1664795 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1664795 ]

          SOLR-7073: test files required for the feature

          Show
          ASF subversion and git services added a comment - Commit 1664795 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1664795 ] SOLR-7073 : test files required for the feature
          Hide
          Noble Paul added a comment -

          final patch

          Show
          Noble Paul added a comment - final patch
          Hide
          ASF subversion and git services added a comment -

          Commit 1664797 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1664797 ]

          SOLR-7073: Add an API to add a jar to a collection's classpath

          Show
          ASF subversion and git services added a comment - Commit 1664797 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1664797 ] SOLR-7073 : Add an API to add a jar to a collection's classpath
          Hide
          ASF subversion and git services added a comment -

          Commit 1665063 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1665063 ]

          SOLR-7073: rename the .jar files to .jar.bin so that the build scripts don't fail

          Show
          ASF subversion and git services added a comment - Commit 1665063 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1665063 ] SOLR-7073 : rename the .jar files to .jar.bin so that the build scripts don't fail
          Hide
          ASF subversion and git services added a comment -

          Commit 1665076 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1665076 ]

          SOLR-7073: renamed PluginRegistry to PluginBag

          Show
          ASF subversion and git services added a comment - Commit 1665076 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1665076 ] SOLR-7073 : renamed PluginRegistry to PluginBag
          Hide
          ASF subversion and git services added a comment -

          Commit 1665228 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1665228 ]

          SOLR-7073: Add an API to add a jar to a collection's classpath, adding test files

          Show
          ASF subversion and git services added a comment - Commit 1665228 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1665228 ] SOLR-7073 : Add an API to add a jar to a collection's classpath, adding test files
          Hide
          ASF subversion and git services added a comment -

          Commit 1665295 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1665295 ]

          SOLR-7073: Support adding a jar to a collections classpath

          Show
          ASF subversion and git services added a comment - Commit 1665295 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1665295 ] SOLR-7073 : Support adding a jar to a collections classpath
          Hide
          ASF subversion and git services added a comment -

          Commit 1666771 from Noble Paul in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1666771 ]

          SOLR-7073: accidentally deleted previos changes

          Show
          ASF subversion and git services added a comment - Commit 1666771 from Noble Paul in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1666771 ] SOLR-7073 : accidentally deleted previos changes
          Hide
          Yonik Seeley added a comment -

          When I saw that this issue had changed how plugins (and lazy plugins) were loaded, I quickly checked to see if concurrency was still handled correctly for lazy loaded. At first blush, it looks like a double-checked-locking race has been introduced. I'll open another issue for this.

          We need to be careful when changes have concurrency implications. Are there other areas of these changes that should be reviewed?

          Show
          Yonik Seeley added a comment - When I saw that this issue had changed how plugins (and lazy plugins) were loaded, I quickly checked to see if concurrency was still handled correctly for lazy loaded. At first blush, it looks like a double-checked-locking race has been introduced. I'll open another issue for this. We need to be careful when changes have concurrency implications. Are there other areas of these changes that should be reviewed?
          Hide
          ASF subversion and git services added a comment -

          Commit 1667799 from Yonik Seeley in branch 'dev/trunk'
          [ https://svn.apache.org/r1667799 ]

          SOLR-7262: fix broken thread safety for request handler registry introduced by SOLR-7073

          Show
          ASF subversion and git services added a comment - Commit 1667799 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1667799 ] SOLR-7262 : fix broken thread safety for request handler registry introduced by SOLR-7073
          Hide
          ASF subversion and git services added a comment -

          Commit 1667803 from Yonik Seeley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1667803 ]

          SOLR-7262: fix broken thread safety for request handler registry introduced by SOLR-7073

          Show
          ASF subversion and git services added a comment - Commit 1667803 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1667803 ] SOLR-7262 : fix broken thread safety for request handler registry introduced by SOLR-7073
          Hide
          Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          Timothy Potter added a comment - Bulk close after 5.1 release

            People

            • Assignee:
              Noble Paul
              Reporter:
              Noble Paul
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development