[SCB-636] change config mapping machanism #745
Conversation
|
try to write config mapping in CSE, not add logic in ServiceComb |
| * | ||
| * To adapte different Platforms | ||
| */ | ||
| public interface PlatformAdaptor { |
There was a problem hiding this comment.
- Maybe we can have a more specific name, e.g. ConfigCenterAddressFinder
- This implementation has coupled config module with config center module & service center module. It's not very good. I think this is config center, service center specific features. Or you need also to implement config-apollo and other modules
- If you are using SPI, it's better to add an getOrder method and can resolve conflicts easier. And add a param or new method to distinguish the usage of the address, e.g. for config center, service center or apollo
jeho0815
force-pushed
the
Branch_master_SCB-636
branch
from
db29fef
to
72805d4
Compare
jeho0815
changed the title
| configMap.putAll(YAMLUtil.yaml2Properties(url.openStream())); | ||
| } | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
i'm sorry~, will change
| ## limitations under the License. | ||
| ## --------------------------------------------------------------------------- | ||
|
|
||
| cse: |
There was a problem hiding this comment.
We are supposed to use servicecomb instead of cse here.
| ## limitations under the License. | ||
| ## --------------------------------------------------------------------------- | ||
|
|
||
| servicecomb: |
There was a problem hiding this comment.
mapping "a" to "b" is
a: b
not
b: a
| private static void duplicateServiceCombConfigToCseAtFront(ConcurrentCompositeConfiguration compositeConfiguration, | ||
| AbstractConfiguration source, | ||
| String sourceName) { | ||
| duplicateServiceCombConfigToCse(source); |
There was a problem hiding this comment.
duplicateServiceCombConfigToCse has already addConfiguration, and here addConfigurationAtFront again, is it necessary to add it twice?
There was a problem hiding this comment.
mapping
ENV: servicecomb.abc
we can use cse.abc to get the value
| urlList.add(urls.nextElement()); | ||
| } | ||
| for (URL url : urlList) { | ||
| configMap.putAll(YAMLUtil.yaml2Properties(url.openStream())); |
There was a problem hiding this comment.
From YAMLUtil.yaml2Properties docs, it's not mentioned if close the stream. Close the stream if necessary or add a api docs that will close internally.
There was a problem hiding this comment.
snake yaml's demo and document don't require to close the stream. but we close the stream maybe more safely
There was a problem hiding this comment.
"snake yaml's demo and document don't require to close the stream"
it's not a good idea to work in this way.
1.YAMLUtil.yaml2Properties is our code
2.if you are not sure if "yaml.loadAll" will close stream, it's better to read the source code, not guess
There was a problem hiding this comment.
yes, i have read the source, i don't find no code to close the stream, but their demo code do not close the stream. Today i write a demo, if i do not close the stream , the stream will be leak.
so add an api document to explain this.
|
too many commits. it's better to make every commit meaningful |
old: A->B just add a new key/value => B/A's value, A has config in microserivce.yaml
new: A->B add a new key/value => A/B's value, B can read from Env/Properties/microservice.yaml, and support multi key map,
A1: B
A2: B
delete the removed config property
code reivew, change the e.printStackTrace() to log and remove the unused code
change the cse to servicecomb
change mapping A->B, support one to multi keys A: - B1 - B2
codereview, safe close the steam
jeho0815
force-pushed
the
Branch_master_SCB-636
branch
from
a0f670b
to
8c2fcca
Compare
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.