Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      See the discussion in SOLR-5955; to properly support collection "templates" that can be specified as the starting point for a collection configuration, we need support for ConfigSets that are immutable.

      1. SOLR-7742
        17 kB
        Gregory Chanan
      2. SOLR-7742.patch
        29 kB
        Gregory Chanan
      3. SOLR-7742log.patch
        0.9 kB
        Gregory Chanan

        Issue Links

          Activity

          Hide
          Gregory Chanan added a comment -

          Here's a patch that implements and tests Immutable ConfigSets. This seems useful by itself (in the case where you don't want the config to be modified), but could have more uses in the future with config templates and/or a ConfigSet API (that e.g. let you copy / instantiate [copy and unset immutable] ConfigSets).

          Note:
          One part I am unsure about is the best way to mark a ConfigSet as being immutable. The solution I chose was to have a file in the configDir named "configSet.immutable". I chose that for three reasons:
          1) It exists outside of the solrconfig / schema, which maps well to what the ConfigSet represents
          2) It can be easily skipped as part of an "instantiate" operation as described above (where we copy a ConfigSet but turn off immutable) via a ConfigSet API call
          3) It works for both local filesystem and ZK configurations (my original idea was to write some properties in the base directory for ZK data, but that wouldn't work for the local filesystem).

          Maybe this would be better:
          We have an optional file named "configSet.properties" that lets you specify (json?) properties that apply to the configset. Today, "immutable" would be the only one that has any meaning, but we could extend this in the future for other properties (e.g. you could have authorization information relevant to a ConfigSet API or specifications like "shareable" [can more than one collection use this as a base]?). The properties file also passes the three motivations listed above.

          Thoughts?

          Show
          Gregory Chanan added a comment - Here's a patch that implements and tests Immutable ConfigSets. This seems useful by itself (in the case where you don't want the config to be modified), but could have more uses in the future with config templates and/or a ConfigSet API (that e.g. let you copy / instantiate [copy and unset immutable] ConfigSets). Note: One part I am unsure about is the best way to mark a ConfigSet as being immutable. The solution I chose was to have a file in the configDir named "configSet.immutable". I chose that for three reasons: 1) It exists outside of the solrconfig / schema, which maps well to what the ConfigSet represents 2) It can be easily skipped as part of an "instantiate" operation as described above (where we copy a ConfigSet but turn off immutable) via a ConfigSet API call 3) It works for both local filesystem and ZK configurations (my original idea was to write some properties in the base directory for ZK data, but that wouldn't work for the local filesystem). Maybe this would be better: We have an optional file named "configSet.properties" that lets you specify (json?) properties that apply to the configset. Today, "immutable" would be the only one that has any meaning, but we could extend this in the future for other properties (e.g. you could have authorization information relevant to a ConfigSet API or specifications like "shareable" [can more than one collection use this as a base] ?). The properties file also passes the three motivations listed above. Thoughts?
          Hide
          Yonik Seeley added a comment -

          We have an optional file named "configSet.properties" that lets you specify (json?) properties that apply to the configset.

          Yeah, I think that's a good idea to plan ahead and prevent a proliferation of files-as-boolean-flags in the future. Then the other decision is whether to expose a generic properties interface (i.e. the ability to set/get arbitrary configset properties).

          The rest of the patch looks good.

          Show
          Yonik Seeley added a comment - We have an optional file named "configSet.properties" that lets you specify (json?) properties that apply to the configset. Yeah, I think that's a good idea to plan ahead and prevent a proliferation of files-as-boolean-flags in the future. Then the other decision is whether to expose a generic properties interface (i.e. the ability to set/get arbitrary configset properties). The rest of the patch looks good.
          Hide
          Gregory Chanan added a comment -

          Yeah, I think that's a good idea to plan ahead and prevent a proliferation of files-as-boolean-flags in the future. Then the other decision is whether to expose a generic properties interface (i.e. the ability to set/get arbitrary configset properties).

          Good points. My thought here is to introduce a ConfigSet API that lets you modify the properties on copy (this is required for immutable, since otherwise a ConfigSet isn't really immutable and you can't use it as a template), but there is no reason we have to limit the modifiable properties to just immutability on copy. As for exposing a generic properties interface outside of copy, that seems reasonable as well, though it's not on my short to-do list. But we could have something like

          /admin/configs/myConfig/properties GET/PUT/POST style interfaces.

          Show
          Gregory Chanan added a comment - Yeah, I think that's a good idea to plan ahead and prevent a proliferation of files-as-boolean-flags in the future. Then the other decision is whether to expose a generic properties interface (i.e. the ability to set/get arbitrary configset properties). Good points. My thought here is to introduce a ConfigSet API that lets you modify the properties on copy (this is required for immutable, since otherwise a ConfigSet isn't really immutable and you can't use it as a template), but there is no reason we have to limit the modifiable properties to just immutability on copy. As for exposing a generic properties interface outside of copy, that seems reasonable as well, though it's not on my short to-do list. But we could have something like /admin/configs/myConfig/properties GET/PUT/POST style interfaces.
          Hide
          Gregory Chanan added a comment -

          Updated version of the patch. Changes:

          1) ConfigSets now support "properties" in the form of a NamedList rather than a single boolean immutable
          2) ConfigSet properties can now be specified in a file called configsetprops.json that is loaded from the resource handler (i.e. same logic as solrconfig/schema, name of file can also be overridden via CoreDescriptor)
          3) JSON parsing and test for ConfigSet properties file; expects a JSON map to be specified

          Note to support 2) above I had to change some error handling around in the resource loader to be able to figure out if a file exists or not (the resource loader throws an IOException if the file can't be found or it can't be open due to permission issues, etc). Since it is okay for a configsetprops.json file not to exist, this seemed like a logical change. Another way to approach this would be to only search for the file in the conf dir (so avoid using the resource loader); I don't really have a preference either way.

          Show
          Gregory Chanan added a comment - Updated version of the patch. Changes: 1) ConfigSets now support "properties" in the form of a NamedList rather than a single boolean immutable 2) ConfigSet properties can now be specified in a file called configsetprops.json that is loaded from the resource handler (i.e. same logic as solrconfig/schema, name of file can also be overridden via CoreDescriptor) 3) JSON parsing and test for ConfigSet properties file; expects a JSON map to be specified Note to support 2) above I had to change some error handling around in the resource loader to be able to figure out if a file exists or not (the resource loader throws an IOException if the file can't be found or it can't be open due to permission issues, etc). Since it is okay for a configsetprops.json file not to exist, this seemed like a logical change. Another way to approach this would be to only search for the file in the conf dir (so avoid using the resource loader); I don't really have a preference either way.
          Hide
          Mark Miller added a comment -

          Did a quick review.

          I had to change some error handling around in the resource loader to be able to figure out if a file exists or not

          That seems fine to me.

          Rest of the patch all looks good to me as well. I'm +1.

          Show
          Mark Miller added a comment - Did a quick review. I had to change some error handling around in the resource loader to be able to figure out if a file exists or not That seems fine to me. Rest of the patch all looks good to me as well. I'm +1.
          Hide
          ASF subversion and git services added a comment -

          Commit 1690828 from gchanan@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1690828 ]

          SOLR-7742: Support for Immutable ConfigSets

          Show
          ASF subversion and git services added a comment - Commit 1690828 from gchanan@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1690828 ] SOLR-7742 : Support for Immutable ConfigSets
          Hide
          ASF subversion and git services added a comment -

          Commit 1690832 from gchanan@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1690832 ]

          SOLR-7742: Support for Immutable ConfigSets

          Show
          ASF subversion and git services added a comment - Commit 1690832 from gchanan@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1690832 ] SOLR-7742 : Support for Immutable ConfigSets
          Hide
          Gregory Chanan added a comment -

          Thanks for the reviews, Mark and Yonik. Committed to trunk and 5.3.

          Show
          Gregory Chanan added a comment - Thanks for the reviews, Mark and Yonik. Committed to trunk and 5.3.
          Hide
          Anshum Gupta added a comment -

          Hi Greg, I've been seeing a lot of Exceptions like the one below after this commit

          org.apache.solr.core.SolrResourceNotFoundException: Can't find resource 'configsetprops.json' in classpath or '/configs/conf1', cwd=/Users/anshumgupta/workspace/lucene-trunk/solr/build/solr-core/test/J0
             [junit4]   2> 	at org.apache.solr.cloud.ZkSolrResourceLoader.openResource(ZkSolrResourceLoader.java:99)
             [junit4]   2> 	at org.apache.solr.core.ConfigSetProperties.readFromResourceLoader(ConfigSetProperties.java:49)
             [junit4]   2> 	at org.apache.solr.core.ConfigSetService.createConfigSetProperties(ConfigSetService.java:114)
             [junit4]   2> 	at org.apache.solr.core.ConfigSetService.getConfig(ConfigSetService.java:76)
             [junit4]   2> 	at org.apache.solr.core.CoreContainer.create(CoreContainer.java:668)
             [junit4]   2> 	at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:397)
             [junit4]   2> 	at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:388)
             [junit4]   2> 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
             [junit4]   2> 	at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:156)
             [junit4]   2> 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
             [junit4]   2> 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
             [junit4]   2> 	at java.lang.Thread.run(Thread.java:745)
          

          P.S: I've noticed this while running the tests generally.

          Show
          Anshum Gupta added a comment - Hi Greg, I've been seeing a lot of Exceptions like the one below after this commit org.apache.solr.core.SolrResourceNotFoundException: Can't find resource 'configsetprops.json' in classpath or '/configs/conf1', cwd=/Users/anshumgupta/workspace/lucene-trunk/solr/build/solr-core/test/J0 [junit4] 2> at org.apache.solr.cloud.ZkSolrResourceLoader.openResource(ZkSolrResourceLoader.java:99) [junit4] 2> at org.apache.solr.core.ConfigSetProperties.readFromResourceLoader(ConfigSetProperties.java:49) [junit4] 2> at org.apache.solr.core.ConfigSetService.createConfigSetProperties(ConfigSetService.java:114) [junit4] 2> at org.apache.solr.core.ConfigSetService.getConfig(ConfigSetService.java:76) [junit4] 2> at org.apache.solr.core.CoreContainer.create(CoreContainer.java:668) [junit4] 2> at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:397) [junit4] 2> at org.apache.solr.core.CoreContainer$1.call(CoreContainer.java:388) [junit4] 2> at java.util.concurrent.FutureTask.run(FutureTask.java:266) [junit4] 2> at org.apache.solr.common.util.ExecutorUtil$MDCAwareThreadPoolExecutor$1.run(ExecutorUtil.java:156) [junit4] 2> at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [junit4] 2> at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [junit4] 2> at java.lang. Thread .run( Thread .java:745) P.S: I've noticed this while running the tests generally.
          Hide
          Noble Paul added a comment -

          Yes I see this error often

          Show
          Noble Paul added a comment - Yes I see this error often
          Hide
          Gregory Chanan added a comment -

          Do you have a specific test case I can look at?

          Show
          Gregory Chanan added a comment - Do you have a specific test case I can look at?
          Hide
          Gregory Chanan added a comment -

          ok, that isn't indicative of an error, it's just logged at info level giving info about where it looked for the properties (it's not an error to not find the properties). I agree it's not ideal because people tend to assume something is serious when they see a stacktrace.

          Show
          Gregory Chanan added a comment - ok, that isn't indicative of an error, it's just logged at info level giving info about where it looked for the properties (it's not an error to not find the properties). I agree it's not ideal because people tend to assume something is serious when they see a stacktrace.
          Hide
          Gregory Chanan added a comment -

          Here's a patch to improve the logging:

          • Makes it more obvious it's not an error, i.e. says "assuming default properties"
          • Only prints the exception message, not the stacktrace
          Show
          Gregory Chanan added a comment - Here's a patch to improve the logging: Makes it more obvious it's not an error, i.e. says "assuming default properties" Only prints the exception message, not the stacktrace
          Hide
          Anshum Gupta added a comment -

          LGTM, thanks Greg. Minor, but this would be a better user experience.

          Show
          Anshum Gupta added a comment - LGTM, thanks Greg. Minor, but this would be a better user experience.
          Hide
          Noble Paul added a comment -

          More often than not configsetprops.json is not present . So why surprise users with any message Just drop the message altogether

          Show
          Noble Paul added a comment - More often than not configsetprops.json is not present . So why surprise users with any message Just drop the message altogether
          Hide
          Mark Miller added a comment -

          +1 - I think logging it is useful as long as it's clear it's an expected situation and the logging is really just for user and dev debugging.

          Show
          Mark Miller added a comment - +1 - I think logging it is useful as long as it's clear it's an expected situation and the logging is really just for user and dev debugging.
          Hide
          Anshum Gupta added a comment -

          +1 on logging it.

          Show
          Anshum Gupta added a comment - +1 on logging it.
          Hide
          ASF subversion and git services added a comment -

          Commit 1692309 from gchanan@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1692309 ]

          SOLR-7742: Avoid stacktrace in info logging

          Show
          ASF subversion and git services added a comment - Commit 1692309 from gchanan@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1692309 ] SOLR-7742 : Avoid stacktrace in info logging
          Hide
          ASF subversion and git services added a comment -

          Commit 1692310 from gchanan@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1692310 ]

          SOLR-7742: Avoid stacktrace in info logging

          Show
          ASF subversion and git services added a comment - Commit 1692310 from gchanan@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1692310 ] SOLR-7742 : Avoid stacktrace in info logging
          Hide
          Gregory Chanan added a comment -

          Thanks for the feedback, committed to trunk and 5x.

          Show
          Gregory Chanan added a comment - Thanks for the feedback, committed to trunk and 5x.
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

            People

            • Assignee:
              Gregory Chanan
              Reporter:
              Gregory Chanan
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development