Skip to content

fix config yaml data type conversion bug #8035

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.

Sign up for GitHub

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

Jump to bottom
Merged
wu-sheng merged 6 commits into from
Oct 29, 2021
Merged

fix config yaml data type conversion bug #8035

wu-sheng merged 6 commits into from
Oct 29, 2021

Conversation

@zhyyu
Copy link
Contributor

@zhyyu zhyyu commented Oct 28, 2021 •
edited
Loading

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #8092.
  • Update the CHANGES log.

wu-sheng
wu-sheng reviewed Oct 28, 2021
View reviewed changes
...r/src/main/java/org/apache/skywalking/oap/server/starter/config/ApplicationConfigLoader.java
Comment on lines +131 to +136
Object replaceValue = yaml.load(valueString);
if (replaceValue instanceof String || replaceValue instanceof Integer || replaceValue instanceof Long || replaceValue instanceof Boolean || replaceValue instanceof ArrayList) {
return replaceValue;
} else {
return valueString;
}
Copy link
Member

@wu-sheng wu-sheng Oct 28, 2021

Choose a reason for hiding this comment

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

Could you add UTs to verify and test what is the case here? And please update the changes.md in the root.

Copy link
Contributor Author

@zhyyu zhyyu Oct 29, 2021

Choose a reason for hiding this comment

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

UTs added, changes.md updated.

Copy link
Member

@wu-sheng wu-sheng Oct 29, 2021

Choose a reason for hiding this comment

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

I can't see what is the case following into this else, could you explain a little?

Also, I can't see the case relating to the error case you reported. Could you add it there too?

Copy link
Contributor Author

@zhyyu zhyyu Oct 29, 2021

Choose a reason for hiding this comment

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

the else case occur when there is a config string like animal: pets, yaml will load it as an object, and return replaceValue isn't class String. I review all the application.yaml config, I found that all the config feild type is one of String Integer Long Boolean ArrayList , so I add the if else check.

Copy link
Member

@wu-sheng wu-sheng Oct 29, 2021

Choose a reason for hiding this comment

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

But animal: pets isn't supported for the core or extension, isn't it? If so, we should throw exception here rather than returning an unsupported value.

Copy link
Member

@wu-sheng wu-sheng Oct 29, 2021

Choose a reason for hiding this comment

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

OK, make sense.

Copy link
Contributor Author

@zhyyu zhyyu Oct 29, 2021

Choose a reason for hiding this comment

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

Have added special password !AI!3B UT for related the error case issue.

@wu-sheng wu-sheng requested a review from JaredTan95 October 28, 2021 07:58
@wu-sheng wu-sheng added backend OAP backend related. bug Something isn't working and you are sure it's a bug! labels Oct 28, 2021
@wu-sheng wu-sheng linked an issue Oct 28, 2021 that may be closed by this pull request
[Bug] config env placeholder yaml parse error #8029
Closed
3 tasks
@wu-sheng
Copy link
Member

wu-sheng commented Oct 29, 2021

Could you fix the CI?

@zhyyu
Copy link
Contributor Author

zhyyu commented Oct 29, 2021

Could you fix the CI?

I have deleted UT empty line and passed checkstyle plugin, maven package success locally.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 29, 2021 

Error:  Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.447 s <<< FAILURE! - in org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase
Error:  testLoadIntegerTypeConfig(org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase)  Time elapsed: 0.995 s  <<< FAILURE!
java.lang.AssertionError: expected:<9941> but was:<9411>
	at org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase.testLoadIntegerTypeConfig(ApplicationConfigLoaderTestCase.java:76)

Error:  Failures: 
Error:    ApplicationConfigLoaderTestCase.testLoadIntegerTypeConfig:76 expected:<9941> but was:<9411>
Error:  Tests run: 6, Failures: 1, Errors: 0, Skipped: 0

Notice, test still fails.

@zhyyu
Copy link
Contributor Author

zhyyu commented Oct 29, 2021 

Error:  Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.447 s <<< FAILURE! - in org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase
Error:  testLoadIntegerTypeConfig(org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase)  Time elapsed: 0.995 s  <<< FAILURE!
java.lang.AssertionError: expected:<9941> but was:<9411>
	at org.apache.skywalking.oap.server.starter.config.ApplicationConfigLoaderTestCase.testLoadIntegerTypeConfig(ApplicationConfigLoaderTestCase.java:76)

Error:  Failures: 
Error:    ApplicationConfigLoaderTestCase.testLoadIntegerTypeConfig:76 expected:<9941> but was:<9411>
Error:  Tests run: 6, Failures: 1, Errors: 0, Skipped: 0

Notice, test still fails.

Sry, type error. Now test all success.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 29, 2021

Just a confirm, is this case failing in the master branch codes? Have you tested that?

@wu-sheng wu-sheng added this to the 8.9.0 milestone Oct 29, 2021
wu-sheng
wu-sheng approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Checked locally, this UT fails before this fix.

@wu-sheng wu-sheng requested a review from kezhenxu94 October 29, 2021 11:43
kezhenxu94
kezhenxu94 approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng merged commit 149b359 into apache:master Oct 29, 2021
kezhenxu94 pushed a commit to kezhenxu94/skywalking that referenced this pull request May 11, 2023
@zhyyu @toffentoffen
fix config yaml data type conversion bug (apache#8035)
39dbcc0 
Co-authored-by: yuzhongyu <yuzhongyu@cestc.cn>
(cherry picked from commit 149b359)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

@JaredTan95 JaredTan95 Awaiting requested review from JaredTan95

Assignees

No one assigned

Labels

backend OAP backend related. bug Something isn't working and you are sure it's a bug!

Projects

None yet

Milestone

8.9.0

Development

Successfully merging this pull request may close these issues.

[Bug] config env placeholder yaml parse error

3 participants

@zhyyu @wu-sheng @kezhenxu94