[SCB-671]duplicate cse to servicecomb #787
Conversation
| } | ||
|
|
||
| public void loadAndSort() { | ||
| try { | ||
| String configFileFromClasspath = | ||
| System.getProperty("cse.configurationSource.defaultFileName") == null ? DEFAULT_CONFIG_FILE_NAME | ||
| : System.getProperty("cse.configurationSource.defaultFileName"); | ||
| System.getProperty(DEFAULT_FILE_NAME) == null ? DEFAULT_CONFIG_FILE_NAME |
There was a problem hiding this comment.
not compatible?
because we only copy config items in configuration source, not in system.property?
There was a problem hiding this comment.
Yes. Intended, we have never mention this in docs. And I add new docs for this change. See java-chassis-doc PR.
There was a problem hiding this comment.
yes, nobody use this feature, we can changed it directly
There was a problem hiding this comment.
Throw an error for this wrong configuration and force users change to correct key.
| @@ -28,7 +28,9 @@ | |||
| public class MicroserviceConfigLoader extends YAMLConfigLoader { | |||
| private static final Logger LOGGER = LoggerFactory.getLogger(MicroserviceConfigLoader.class); | |||
|
|
|||
| private static final String ADDITIONAL_CONFIG_URL = "cse.configurationSource.additionalUrls"; | |||
| private static final String ADDITIONAL_CONFIG_URL = "servicecomb.configurationSource.additionalUrls"; | |||
There was a problem hiding this comment.
not compatible?
because we only copy config items in configuration source, not in system.property?
There was a problem hiding this comment.
i remember someone already used this feature
There was a problem hiding this comment.
This property is described in the README file, we need to make sure the old setting works.
There was a problem hiding this comment.
Throw an error for this wrong configuration and force users change to correct key.
There was a problem hiding this comment.
I deleted the README file and add the contents to java-chassis-doc, please see that PR.
| //inject a copy of cse.xxx for servicecomb.xxx | ||
| private static void duplicateServiceCombConfigToCse(AbstractConfiguration source) { | ||
| //inject a copy of servicecomb.xxx for cse.xxx | ||
| private static void duplicateCseConfigToServicecomb(AbstractConfiguration source) { |
There was a problem hiding this comment.
Just a quick question，if the user has two value setting for servicecomb.xxx and cse.xxx, which one will take effect？
We may need add some document for it.
There was a problem hiding this comment.
Depends on servicecomb-config-order, and I add new test cased for this in new commit. And adding warning message if two keys are in different files.
| // compatible check | ||
| objOrder = config.get("cse-config-order"); | ||
| if (objOrder != null) { | ||
| LOGGER.error("cse-config-order will not be supported in future, please change it to servicecomb-config-order"); |
There was a problem hiding this comment.
I think it should be warning, as the program is not stop working here.
There was a problem hiding this comment.
This is quick fix for users, so I give error not warning. Users see this, it quite important to fix it immediately. Some others like above duplicate key, I give warning, as users can not change common modules.
I am thinking of throw an exception here, just like cse.configurationSource.additionalUrls do, but after quite a lot consideration, give an error message is better.
…er&add compatible error to config property
|
I merge this PR first. Because this blocking other changes |
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean installto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.