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

Code Simplification, Structure Changes, and Translation Work, Along with a Question #1827

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

dukbong
Copy link
Contributor

@dukbong dukbong commented Apr 24, 2024

What's changed?

Description

This PR simplifies the code by using Assert methods and changes the location of the validateParams() process.
By moving this process, validateParams() can perform a null check earlier.

Question

In the JmxCollectImpl code, the getConnectSession() checks whether jmxProtocol.getUrl() != null, which is already validated in validateParams().
This redundancy seems unnecessary, but I'm not entirely sure if I'm missing something, so I left the source code unchanged.
Do you think this extra check is required?

Other Work

This PR also includes translation work, converting text from Chinese to English.

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@tomsun28
Copy link
Contributor

hi this is not redundancy. jmxProtocol.getUrl() is user cutom url, can equlas null.
The validateParams process is to check url whether has sepcial word when it not null.

@dukbong
Copy link
Contributor Author

dukbong commented Apr 24, 2024

hi this is not redundancy. jmxProtocol.getUrl() is user cutom url, can equlas null. The validateParams process is to check url whether has sepcial word when it not null.

Thanks for confirming!
I had a feeling there might be something more to it, but I wasn't sure, so I wanted to ask.

@tomsun28 tomsun28 merged commit 54c163a into apache:master Apr 24, 2024
3 checks passed
@dukbong dukbong deleted the change branch April 24, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants