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

Issue 9130: Pulsar-admin sinks create: bad error message "java.lang.NullPointerException: path is 'null'." in case of missing "--name" parameter #9131

Merged
merged 3 commits into from
Jan 12, 2021

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Jan 5, 2021

Fixes #9130

Motivation

Return a better error to the user

Modifications

Add an explicit validation and do not let WebTarget#path throw an internal error.
The validation covers tenant,namespace and sinkname in all of the functions regarding sinks.
We must do the validation here because we have to prevent code to pass bad values to JAX-RS client

Verifying this change

This change added unit tests

…ullPointerException: path is 'null'." in case of missing "--name" parameter
@@ -232,6 +233,10 @@ public void createSink(SinkConfig sinkConfig, String fileName) throws PulsarAdmi
@Override
public CompletableFuture<Void> createSinkAsync(SinkConfig sinkConfig, String fileName) {
final CompletableFuture<Void> future = new CompletableFuture<>();
if (StringUtils.isBlank(sinkConfig.getName())) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a better story here to validate the parameters? For example, what if tenant or namespace is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sijie I have added validation for all of the methods in SinksImpl for sinkname, namespace and tenant.
PTAL

@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-tests

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie
Copy link
Member

sijie commented Jan 11, 2021

@eolivelli Can you rebase to the latest master?

@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@eolivelli
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie merged commit 7b48dea into apache:master Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants