-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-37427] Fix incorrect NPE message in StandaloneHaServices constructor #26258
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
base: master
Are you sure you want to change the base?
Conversation
Replace checkNotNull(clusterRestEndpointAddress, clusterRestEndpointAddress) with checkNotNull(clusterRestEndpointAddress, "clusterRestEndpointAddress") to ensure the exception message clearly identifies the null parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could we have a unit test, to check that null values are handled as expected. As this is a trivial change - it is your call as to whether you add this.
@@ -61,7 +61,7 @@ public StandaloneHaServices( | |||
checkNotNull(resourceManagerAddress, "resourceManagerAddress"); | |||
this.dispatcherAddress = checkNotNull(dispatcherAddress, "dispatcherAddress"); | |||
this.clusterRestEndpointAddress = | |||
checkNotNull(clusterRestEndpointAddress, clusterRestEndpointAddress); | |||
checkNotNull(clusterRestEndpointAddress, "clusterRestEndpointAddress"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could correct the javadoc and add this parameter while we are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving
- raised some nits but these are nice to haves and the change looks good.
This PR is being marked as stale since it has not had any activity in the last 90 days. If you are having difficulty finding a reviewer, please reach out to the If this PR is no longer valid or desired, please feel free to close it. |
What is the purpose of the change
developers receive unhelpful error messages when parameters are null.
Brief change log
Replace checkNotNull(clusterRestEndpointAddress, clusterRestEndpointAddress) with checkNotNull(clusterRestEndpointAddress, "clusterRestEndpointAddress") to ensure the exception message clearly identifies the null parameter.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation