SOLR-18167: Fix the original PR #4507 to cover more use cases in mapping old to new system property names#4535
SOLR-18167: Fix the original PR #4507 to cover more use cases in mapping old to new system property names#4535epugh wants to merge 2 commits into
Conversation
|
@utsav00 I'd love your review of this fix! |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
why did Jenkins not run that test?
| } | ||
| for (String key : deprecatedProps.stringPropertyNames()) { | ||
| DEPRECATED_MAPPINGS.put(deprecatedProps.getProperty(key), key); | ||
| // The lookup side (findDeprecatedMappingKey) normalises incoming "-D" keys to |
There was a problem hiding this comment.
| // The lookup side (findDeprecatedMappingKey) normalises incoming "-D" keys to | |
| // The lookup side (findDeprecatedMappingKey) normalizes system properties to |
Would that be just as correct?
| public void deprecatedCamelCaseOldNameInMappingsFileIsTranslated() { | ||
| // Here the *old* name in the mappings file is itself camelCase (collection.configName), unlike | ||
| // deprecatedCamelCaseDFlagIsTranslatedToCurrentPropertyName where the file uses a dot-separated | ||
| // old name. Passing the camelCase old name verbatim as a -D flag must still be migrated. See |
There was a problem hiding this comment.
In general, as a Java developer, I simply say "system property". I don't say "-D" flag... while yes I understand that is the argument to Java to pass them in on startup.
| // The lookup side (findDeprecatedMappingKey) normalises incoming "-D" keys to | ||
| // dot-separated form, so normalise the old (camelCase) names here too | ||
| DEPRECATED_MAPPINGS.put(camelCaseToDotSeparated(deprecatedProps.getProperty(key)), key); |
There was a problem hiding this comment.
Oo, I like this idea more. I was going for a redundant check that required an extra guard - https://github.com/apache/solr/compare/main...utsav00:solr:fix/SOLR-18167?expand=1
utsav00
left a comment
utsav00
left a comment
There was a problem hiding this comment.
LGTM! Thanks for fixing my fix!
There was a problem hiding this comment.
Not fix blocking but some small refactor: With this load time lookup normalization, we can remove these methods that were added in the initial fix and the original loop can be simplified again to
| var dotKey = camelCaseToDotSeparated(deprecatedKey); | |
| if (DEPRECATED_MAPPINGS.containsKey(dotKey) || DEPRECATED_MAPPINGS.containsKey("!" + dotKey)) { | |
| applyDeprecatedPropertyMapping(deprecatedKey, dotKey, sysProperties); | |
| } |
| } | ||
|
|
||
| @Test | ||
| public void deprecatedCamelCaseOldNameInMappingsFileIsTranslated() { |
There was a problem hiding this comment.
This looks good. Can we also add a test for deprecated inverted camel case property to cover all the cases?
|
@utsav00 you have some good ideas, do you want to push to my branch? I just added you as a collaborator on |
looks like it only runs "test" for |
TST: add another test case
|
@utsav00 thanks for the commit... Any other changes you want? I'll run the tests and then merge.. |
4 participants
https://issues.apache.org/jira/browse/SOLR-18167
Description
test_start_solr.batsis failing!Solution
Turns out the original PR didn't cover all the uses, and unfortuanntly the bats tests didn't get run by Github. Also, the nightly jenkins don't run them.
This fixes it, and adds another Java level unit test to verify the fix.
Tests
Update bats, add new JUnit.
cc @utsav00