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/08/30 07:57:11 UTC
[GitHub] [skywalking] wankai123 opened a new pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
wankai123 opened a new pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [X] Explain briefly why the bug exists and how to fix it.
- [ ] 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).
In `ConfigWatcherRegister.singleConfigsSync()`, when config is deleted and `watcher.value()` is not null, core would notify watcher every syncPeriod time.
``` java
ConfigChangeWatcher watcher = holder.getWatcher();
String newItemValue = item.getValue();
if (newItemValue == null) {
if (watcher.value() != null) {
// Notify watcher, the new value is null with delete event type.
watcher.notify(
new ConfigChangeWatcher.ConfigChangeEvent(null, ConfigChangeWatcher.EventType.DELETE));
} else {
// Don't need to notify, stay in null.
}
```
--
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 #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698312770
##########
File path: oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
##########
@@ -62,13 +61,15 @@ private void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
this.settingsString.set(config);
- onGatewaysUpdated(parseGatewaysFromYml(config));
+ if (config != null) {
+ onGatewaysUpdated(parseGatewaysFromYml(""));
+ }
Review comment:
yes, mistake that cause ci failed
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698303912
##########
File path: oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
##########
@@ -62,13 +61,15 @@ private void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
this.settingsString.set(config);
- onGatewaysUpdated(parseGatewaysFromYml(config));
+ if (config != null) {
+ onGatewaysUpdated(parseGatewaysFromYml(""));
+ }
Review comment:
You might mean `if (config == null)` update with `""`? The same in other places in this PR
```suggestion
if (config == null) {
onGatewaysUpdated(parseGatewaysFromYml(""));
}
```
--
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] JaredTan95 merged pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
JaredTan95 merged pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606
--
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 #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698296938
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/logging/LoggingConfigWatcher.java
##########
@@ -63,14 +64,6 @@ public void notify(final ConfigChangeEvent value) {
log.error("failed to apply configuration to log4j", t);
return;
}
- StringBuilder builder = new StringBuilder();
- ctx.getConfiguration().getLoggers().forEach((loggerName, config) -> {
- builder.append(Strings.isNullOrEmpty(loggerName) ? "Root" : loggerName)
- .append(":")
- .append(config.getLevel())
- .append(",");
- });
- this.content = builder.toString();
Review comment:
@hanahmily This was a bug of old codes, which cause endless loop of update notification.
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698317734
##########
File path: oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
##########
@@ -62,13 +61,17 @@ private void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
this.settingsString.set(config);
- onGatewaysUpdated(parseGatewaysFromYml(config));
+ if (config != null) {
+ onGatewaysUpdated(parseGatewaysFromYml(config));
+ } else {
+ onGatewaysUpdated(parseGatewaysFromYml(""));
+ }
Review comment:
The same as other places
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698284166
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/ApdexThresholdConfig.java
##########
@@ -90,7 +89,9 @@ private synchronized void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
rawConfig = config;
- updateConfig(new StringReader(config));
+ if (config != null) {
+ updateConfig(new StringReader(config));
+ }
Review comment:
`config == ""` was a signal to "reset" the config, with this new `if` statement, the config might never be reset even the dynamic config is deleted?
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698284166
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/ApdexThresholdConfig.java
##########
@@ -90,7 +89,9 @@ private synchronized void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
rawConfig = config;
- updateConfig(new StringReader(config));
+ if (config != null) {
+ updateConfig(new StringReader(config));
+ }
Review comment:
`config == ""` was an signal to "reset" the config, with this new `if` statement, the config might never been reset even the dynamic config is deleted?
--
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 #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698295141
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/ApdexThresholdConfig.java
##########
@@ -90,7 +89,9 @@ private synchronized void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
rawConfig = config;
- updateConfig(new StringReader(config));
+ if (config != null) {
+ updateConfig(new StringReader(config));
+ }
Review comment:
fixed
--
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 pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#issuecomment-908792905
@JaredTan95 when you do merging in the next time, please make the commit logs clean.
```
Fix dynamic configuration watch implementation current value not null…
… when the config is deleted. (#7606)
* Fix dynamic configuration watch implementation current value not null when the config is deleted.
* add changes.md
* fix LoggingConfigWatcher watch.vale, and when value = null ,config not reset.
* fix
* fix style
* Update oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
Co-authored-by: kezhenxu94 <ke...@apache.org>
* use Strings.nullToEmpty
Co-authored-by: 吴晟 Wu Sheng <wu...@foxmail.com>
Co-authored-by: kezhenxu94 <ke...@apache.org>
```
This is how it looks like now. Too many unnecessary things.
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698284166
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/ApdexThresholdConfig.java
##########
@@ -90,7 +89,9 @@ private synchronized void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
rawConfig = config;
- updateConfig(new StringReader(config));
+ if (config != null) {
+ updateConfig(new StringReader(config));
+ }
Review comment:
`config == ""` was an signal to "reset" the config, with this new `if` statement, the config might never be reset even the dynamic config is deleted?
--
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 pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#issuecomment-908792905
@JaredTan95 when you do merging in the next time, please make the commit logs clean.
```
Fix dynamic configuration watch implementation current value not null…
… when the config is deleted. (#7606)
* Fix dynamic configuration watch implementation current value not null when the config is deleted.
* add changes.md
* fix LoggingConfigWatcher watch.vale, and when value = null ,config not reset.
* fix
* fix style
* Update oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
Co-authored-by: kezhenxu94 <ke...@apache.org>
* use Strings.nullToEmpty
Co-authored-by: 吴晟 Wu Sheng <wu...@foxmail.com>
Co-authored-by: kezhenxu94 <ke...@apache.org>
```
This is how it looks like now. Too many unnecessary things.
--
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] kezhenxu94 commented on a change in pull request #7606: Fix dynamic configuration watch implementation current value not null when the config is deleted.
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7606:
URL: https://github.com/apache/skywalking/pull/7606#discussion_r698314849
##########
File path: oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/UninstrumentedGatewaysConfig.java
##########
@@ -62,13 +61,17 @@ private void activeSetting(String config) {
log.debug("Updating using new static config: {}", config);
}
this.settingsString.set(config);
- onGatewaysUpdated(parseGatewaysFromYml(config));
+ if (config != null) {
+ onGatewaysUpdated(parseGatewaysFromYml(config));
+ } else {
+ onGatewaysUpdated(parseGatewaysFromYml(""));
+ }
Review comment:
nit: we can leverage `com.google.common.base.Strings.nullToEmpty(config)`
```suggestion
onGatewaysUpdated(parseGatewaysFromYml(Strings.nullToEmpty(config)));
```
--
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