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

MINOR: Catch null pointer exception for empty leader URL when assignment is null #4798

Merged
merged 1 commit into from Nov 16, 2018
Merged

MINOR: Catch null pointer exception for empty leader URL when assignment is null #4798

merged 1 commit into from Nov 16, 2018

Conversation

@asdf2014
Copy link
Member

@asdf2014 asdf2014 commented Mar 30, 2018

Catch null pointer exception for empty leader URL when assignment is null.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
@asdf2014 asdf2014 changed the title MINRO: Catch null pointer exception for empty leader URL when assignment is null MINOR: Catch null pointer exception for empty leader URL when assignment is null Mar 30, 2018
@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented Apr 4, 2018

Hi, @guozhangwang . PTAL.

@guozhangwang
Copy link
Contributor

@guozhangwang guozhangwang commented Apr 4, 2018

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Thanks @asdf2014 for spotting this. I left a comment.

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated
@@ -1016,7 +1016,7 @@ public void run() {
String reconfigUrl = RestServer.urlJoin(leaderUrl(), "/connectors/" + connName + "/tasks");
RestClient.httpRequest(reconfigUrl, "POST", taskProps, null, config);
cb.onCompletion(null, null);
} catch (ConnectException e) {
} catch (ConnectException | NullPointerException e) {

This comment has been minimized.

@kkonstantine

kkonstantine Apr 4, 2018
Contributor

I'd rather not use exception handling for branching. I think I'd prefer to address this case with an earlier check for leaderUrl() == null and instantiate a more informative exception to pass to cb.onCompletion

@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented Apr 8, 2018

Hi, @kkonstantine @guozhangwang . Already using more informative exception instead of NullPointerException. PTAL.

Copy link
Contributor

@rhauch rhauch left a comment

One minor suggestion to make it even more readable.

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated
@@ -1013,7 +1013,12 @@ private void reconfigureConnector(final String connName, final Callback<Void> cb
@Override
public void run() {
try {
String reconfigUrl = RestServer.urlJoin(leaderUrl(), "/connectors/" + connName + "/tasks");
String leaderUrl = leaderUrl();
if (leaderUrl == null || leaderUrl.trim().length() == 0)

This comment has been minimized.

@rhauch

rhauch Apr 8, 2018
Contributor

Maybe leaderUrl.trim().isEmpty() instead?

This comment has been minimized.

@asdf2014

asdf2014 Apr 9, 2018
Author Member

Indeed, isEmpty() is better.

Copy link
Contributor

@kkonstantine kkonstantine left a comment

I left a few minor comments to improve the exception message and the readability of the code.

Finally, an observation I have is that we now throw an exception which we know it should (and will) be caught right below. Another way to write this, would be to pass the exception to cb.onCompletion immediately. E.g.

  Exception e = new ConnectException(...);
  cb.onCompletion(e, null);

Since this is not expected to have an impact on the efficiency of the code here, this is more of a stylistic comment than anything else. Up to you @asdf2014

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated
String leaderUrl = leaderUrl();
if (leaderUrl == null || leaderUrl.trim().isEmpty())
throw new ConnectException("Request to leader to " +
"reconfigure connector tasks failed, " +

This comment has been minimized.

@kkonstantine

kkonstantine Apr 9, 2018
Contributor

nit: , (comma) doesn't seem required in the sentence.

This comment has been minimized.

@asdf2014

asdf2014 Apr 9, 2018
Author Member

All right. I remove it :-)

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated
if (leaderUrl == null || leaderUrl.trim().isEmpty())
throw new ConnectException("Request to leader to " +
"reconfigure connector tasks failed, " +
"due to the URL for the leader's REST interface is empty!");

This comment has been minimized.

@kkonstantine

kkonstantine Apr 9, 2018
Contributor

suggestion: because the URL of the leader's REST interface is empty
(otherwise it should have been due to the URL of the leader's REST interface being empty)

This comment has been minimized.

@asdf2014

asdf2014 Apr 9, 2018
Author Member

Yep, you are right. It is better.

...ct/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java Outdated
@@ -1013,7 +1013,12 @@ private void reconfigureConnector(final String connName, final Callback<Void> cb
@Override
public void run() {
try {
String reconfigUrl = RestServer.urlJoin(leaderUrl(), "/connectors/" + connName + "/tasks");
String leaderUrl = leaderUrl();
if (leaderUrl == null || leaderUrl.trim().isEmpty())

This comment has been minimized.

@kkonstantine

kkonstantine Apr 9, 2018
Contributor

Although the checkstyle rules currently do not enforce curly brackets in if/else blocks that contain a single statement, because this statement here spans multiple lines, I feel it'd be best to enclose it within { }, even if that's optional.

This comment has been minimized.

@asdf2014

asdf2014 Apr 9, 2018
Author Member

Sure, i will follow the code style.

@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented Apr 9, 2018

Thanks for your suggestions. Already fixed. @kkonstantine

…signment is null

* Using more informative exception instead of `NullPointerException`

* Using isEmpty instead of `lenght == 0`

* Add brackets for if clause and improve the connect exception message
@asdf2014
Copy link
Member Author

@asdf2014 asdf2014 commented Nov 16, 2018

Hi, @rhauch @kkonstantine . This PR has been squashed and rebased. PTAL.

@rhauch
rhauch approved these changes Nov 16, 2018
Copy link
Contributor

@rhauch rhauch left a comment

LGTM. Thanks, @asdf2014!

@guozhangwang guozhangwang merged commit b21e933 into apache:trunk Nov 16, 2018
2 checks passed
2 checks passed
JDK 11 and Scala 2.12 SUCCESS 10175 tests run, 5 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 10175 tests run, 5 skipped, 0 failed.
Details
@asdf2014 asdf2014 deleted the asdf2014:null_leader_url branch Nov 17, 2018
Pengxiaolong added a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…signment is null (apache#4798)

Catch null pointer exception for empty leader URL when assignment is null.

Reviewers: Randall Hauch <rhauch@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants