You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/09/01 07:29:31 UTC
[GitHub] [skywalking] wankai123 opened a new pull request #7632: Support nacos grouped dynamic configurations.
wankai123 opened a new pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632
### Support nacos grouped dynamic configurations.
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [X] Update the documentation to include this new feature.
- [X] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
See `dynamic-config-nacos.md`
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a change in pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632#discussion_r700059866
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
+ keys1.add("test-module.default.testKeyGroup");
Review comment:
sorry, forgot to remove it...
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632#discussion_r699958244
##########
File path: docs/en/setup/backend/dynamic-config-nacos.md
##########
@@ -18,4 +18,64 @@ configuration:
period: ${SW_CONFIG_NACOS_PERIOD:60}
# the name of current cluster, set the name if you want to upstream system known.
clusterName: ${SW_CONFIG_NACOS_CLUSTER_NAME:default}
-```
\ No newline at end of file
+```
+
+## Config Storage
+### Single Config
+
+| Data Id | Group | Config Value |
+|-----|-----|-----|
+| configKey | {group} | configValue |
+
+e.g. The config is:
+```
+{agent-analyzer.default.slowDBAccessThreshold}:{default:200,mongodb:50}
+```
+If `group = skywalking` the config in nacos is:
+
+| Data Id | Group | Config Value |
+|-----|-----|-----|
+| agent-analyzer.default.slowDBAccessThreshold | skywalking | default:200,mongodb:50 |
+
+### Group Config
+
+| Data Id | Group | Config Value | Config Type |
+|-----|-----|-----|-----|
+| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
+| subItemkey1 | {group} | subItemValue1 |
+| subItemkey2 | {group} | subItemValue2 |
+| ... | ... | ... |
+
+Notice: If you add/remove a subItem, you need to add/remove the subItemKey from the group which the subItem belongs:
+
+| Data Id | Group | Config Value | Config Type |
+|-----|-----|-----|-----|
+| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
+We separate subItemkeys by `\n` or `\r\n`, trim leading and trailing whitespace, if you set the config by `Nacos UI` each subItemkey should in a new line:
Review comment:
```suggestion
| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
We separate subItemkeys by `\n` or `\r\n`, trim leading and trailing whitespace, if you set the config by `Nacos UI` each subItemkey should in a new line:
```
You need an empty line here. Otherwise, GitHub preview looks like this
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632#discussion_r699958244
##########
File path: docs/en/setup/backend/dynamic-config-nacos.md
##########
@@ -18,4 +18,64 @@ configuration:
period: ${SW_CONFIG_NACOS_PERIOD:60}
# the name of current cluster, set the name if you want to upstream system known.
clusterName: ${SW_CONFIG_NACOS_CLUSTER_NAME:default}
-```
\ No newline at end of file
+```
+
+## Config Storage
+### Single Config
+
+| Data Id | Group | Config Value |
+|-----|-----|-----|
+| configKey | {group} | configValue |
+
+e.g. The config is:
+```
+{agent-analyzer.default.slowDBAccessThreshold}:{default:200,mongodb:50}
+```
+If `group = skywalking` the config in nacos is:
+
+| Data Id | Group | Config Value |
+|-----|-----|-----|
+| agent-analyzer.default.slowDBAccessThreshold | skywalking | default:200,mongodb:50 |
+
+### Group Config
+
+| Data Id | Group | Config Value | Config Type |
+|-----|-----|-----|-----|
+| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
+| subItemkey1 | {group} | subItemValue1 |
+| subItemkey2 | {group} | subItemValue2 |
+| ... | ... | ... |
+
+Notice: If you add/remove a subItem, you need to add/remove the subItemKey from the group which the subItem belongs:
+
+| Data Id | Group | Config Value | Config Type |
+|-----|-----|-----|-----|
+| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
+We separate subItemkeys by `\n` or `\r\n`, trim leading and trailing whitespace, if you set the config by `Nacos UI` each subItemkey should in a new line:
Review comment:
```suggestion
| configKey | {group} | subItemkey1</br>subItemkey2</br>... | TEXT |
We separate subItemkeys by `\n` or `\r\n`, trim leading and trailing whitespace, if you set the config by `Nacos UI` each subItemkey should in a new line:
```
You need an empty line here. Otherwise, GitHub preview looks like this
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
Review comment:
```suggestion
Set<String> keys = new HashSet<>();
```
Is `s` suffix a typo?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a change in pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632#discussion_r700033858
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
+ keys1.add("test-module.default.testKeyGroup");
Review comment:
What is this? Legacy testing codes?
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
Review comment:
```suggestion
```
What is this?
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
+ keys1.add("test-module.default.testKeyGroup");
+
+ GroupConfigTable groupConfigTable = new GroupConfigTable();
+ keys1.forEach(key -> {
Review comment:
`keys1` seems never getting initialized properly, and real `keys` are never used.
I assume you submitted the local testing codes.
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
+ keys1.add("test-module.default.testKeyGroup");
+
+ GroupConfigTable groupConfigTable = new GroupConfigTable();
+ keys1.forEach(key -> {
Review comment:
Even in this case, the UTs are passed. I think UTs miss some important cases.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a change in pull request #7632: Support nacos grouped dynamic configurations.
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #7632:
URL: https://github.com/apache/skywalking/pull/7632#discussion_r700062867
##########
File path: oap-server/server-configuration/configuration-nacos/src/main/java/org/apache/skywalking/oap/server/configuration/nacos/NacosConfigWatcherRegister.java
##########
@@ -92,8 +91,35 @@ public NacosConfigWatcherRegister(NacosServerSettings settings) throws NacosExce
@Override
public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
- // TODO: implement readGroupConfig
- return Optional.empty();
+ Set<String> keys1 = new HashSet<>();
+ keys1.add("test-module.default.testKeyGroup");
+
+ GroupConfigTable groupConfigTable = new GroupConfigTable();
+ keys1.forEach(key -> {
Review comment:
because the `key` is the same as the UTs :(
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org