HoustonPutman commented on code in PR #1314:
URL: https://github.com/apache/solr/pull/1314#discussion_r1088335316
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -1162,4 +1176,19 @@ public ConfigNode get(String name) {
public ConfigNode get(String name, Predicate<ConfigNode> test) {
return root.get(name, test);
}
+
+ /**
+ * Generates a String ID to represent the {@link ConfigSet}
+ *
+ * <p>Relies on the name of the ConfigSet, the name of the SolrConfig, the
{@link
+ * String#hashCode()} to generate a "unique" id for the solr.xml data
(including substitutions),
+ * and the version of the overlay. These 3 peices of data should combine to
make a "unique"
+ * identifier for SolrConfigs, since those are ultimately all inputs to
modifying the solr.xml
+ * result.
+ *
+ * @param configSetName the name of the configSet that this SolrConfig
belongs to.
+ */
+ public String effectiveId(String configSetName) {
+ return configSetName + "-" + getName() + "-" + znodeVersion + "-" +
rootDataHashCode;
+ }
Review Comment:
Yeah the documentation was meant to say SolrConfig, not ConfigSet.
> I'm skeptical configSetName even needs to be a part of this.
I'm also skeptical, but saw no harm in adding it.
> then I think it's the caller of this method to concatenate the configSet
name.
I did this originally then added the configSetName here. In general if we
don't see a use case of sharing SolrConfigs between configSets, then I don't
really see a need to make that a possibility. @gerlowskija can weigh in, but I
don't really want to open that rats nest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
