Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

【SCB-1615】Extended dynamic configuration support for Nacos #1405

Merged
merged 15 commits into from Nov 26, 2019

Conversation

160731liupf
Copy link
Contributor

How to use:
1.Download base model from http://start.servicecomb.io and add maven dependence:

org.apache.servicecomb
config-nacos
2.0.0-SNAPSHOT

2.Start nacos console model(git url:https://github.com/alibaba/nacos) ,both account and password is "nacos" and add properties(example):
Data ID: example
Group:DEFAULT_GROUP
JSON:
{
"nacos":"666"
}
3.In base model,add info in microservice.yaml:
nacos:
config:
serverAddr: 127.0.0.1:8848
dataId: example
group: DEFAULT_GROUP
4.Then add blow code and start base model,you will get properties(If properties on nacos has changed, you can also get new value):
@RestSchema(schemaId = "nacos")
@RequestMapping(path = "/")
public class NacosImpl {
/**

  • 从nacos获取配置
  • @return
    */
    @GetMapping(path = "/config")
    @responsebody
    public String config() throws Exception{
    final String config = DynamicPropertyFactory.getInstance()
    .getStringProperty("nacos","").getValue();
    return JSON.toJSONString(config);
    }
    }

@coveralls
Copy link

coveralls commented Nov 22, 2019

Coverage Status

Coverage decreased (-0.08%) to 85.365% when pulling e0925a5 on 160731liupf:master into 7701260 on apache:master.

Comment on lines 47 to 50
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-jdk14</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you modifiy this? I think can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry,It's my fault.Mybe my project is not full compilation .A few days ago,When I run test in config-apollo,I found it print nothing.There is no need to add this dependency,I had remove it in a new commit.

Comment on lines 55 to 58
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-jdk14</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has removed in a new commit.

Comment on lines 52 to 55
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-jdk14</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does nacos must use jdk logger? ServiceComb use sl4j to log messages, and users can choose from jdk logger, log4j, logback, log4j2, etc. It's better not put this dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had removed it in a new commit

@liubao68
Copy link
Contributor

suggestions: you can add a README.md file for this module, and put your usage guide in this file.

@160731liupf
Copy link
Contributor Author

suggestions: you can add a README.md file for this module, and put your usage guide in this file.

Thanks for your suggestions,I had create a new commit to remove dependency and add README.md file.

@@ -1341,6 +1343,11 @@
<artifactId>business-service-springmvc-archetype</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new libraries need update LICENCE file in ${HOME}/java-chassis-distributioni/release/src/LICENCE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had add
"* nacos-client (com.alibaba.nacos:1.1.4 - https://github.com/alibaba/nacos)"
in LICENSE file.

Comment on lines 106 to 108
}catch (Exception e){
LOGGER.error("Receive nacos config error: ", e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small piece of code is not well formatted using code template in ${HOME}/etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以用中文解释一下吗?我不明白要怎么修改比较好

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有少量代码没有正确格式化(比如空格等),可以使用IDE自动格式化一下。 格式化模板在servicecomb的 etc 目录。 里面包含了eclipse, IDEA等IDE的格式化模板。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你好,config-nacos 模块我已经用intellij-java-google-style.xml格式化好了,这是pr地址:https://github.com/apache/servicecomb-java-chassis/pull/1414/files


package org.apache.servicecomb.config.client;

public enum ConfigurationAction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You used same package and class name with config-appolo. I Suggest you refactor all code to use a different package name for all class. e.g.

org.apache.servicecomb.config.nacos

And we will handle other implentations, maybe in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package name of config-nacos model has replaced with "org.apache.servicecomb.config.nacos".


private List<WatchedUpdateListener> listeners = new CopyOnWriteArrayList<>();

private static final String NACOS_CONFIG_SERVER_ADDR_KEY = "nacos.config.serverAddr";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use the same constant in NacosConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of key is replaced with "SERVER_ADDR".

@liubao68 liubao68 merged commit 20ff2f0 into apache:master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants