Skip to content

Conversation

@TakaHiR07
Copy link
Contributor

Motivation

A fix for #17251. There is an error when handling ConflictException.

The previous unittest can not approve it, so I add another test.

Modifications

ex.getCause() -> ex

add test.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 31, 2022
@TakaHiR07 TakaHiR07 changed the title fix update topic partition fix catching ConflictException when update topic partition Aug 31, 2022
@TakaHiR07
Copy link
Contributor Author

@HQebupt @Jason918 @AnonHxy Please take a look.

@HQebupt
Copy link
Contributor

HQebupt commented Aug 31, 2022

@AnonHxy AnonHxy changed the title fix catching ConflictException when update topic partition [fix][broker]fix catching ConflictException when update topic partition Aug 31, 2022
@AnonHxy
Copy link
Contributor

AnonHxy commented Aug 31, 2022

Nice catch.

The code below hide the exception, that's why the test can pass without throw exception. I think the code below wants to try to solve the same problem as #17251, but has some problem.

I think that we can also remove the code from Line4399-Line4402. Because #17251 fix it better. WDYT @TakaHiR07

}).thenAccept(__ -> result.complete(null)).exceptionally(ex -> {
if (force && ex.getCause() instanceof PulsarAdminException.ConflictException) {
result.complete(null);
return null;
}
result.completeExceptionally(ex);
return null;
});

@TakaHiR07
Copy link
Contributor Author

TakaHiR07 commented Sep 1, 2022

Nice catch.

The code below hide the exception, that's why the test can pass without throw exception. I think the code below wants to try to solve the same problem as #17251, but has some problem.

I think that we can also remove the code from Line4399-Line4402. Because #17251 fix it better. WDYT @TakaHiR07

}).thenAccept(__ -> result.complete(null)).exceptionally(ex -> {
if (force && ex.getCause() instanceof PulsarAdminException.ConflictException) {
result.complete(null);
return null;
}
result.completeExceptionally(ex);
return null;
});

@AnonHxy I think it is good! We do not need this code after #17251.

I have force-push to contain this change.

@AnonHxy AnonHxy requested a review from Jason918 September 1, 2022 03:41
@AnonHxy AnonHxy closed this Sep 13, 2022
@AnonHxy AnonHxy reopened this Sep 13, 2022
@AnonHxy AnonHxy added release/2.11.1 type/bug The PR fixed a bug or issue reported a bug labels Sep 13, 2022
@AnonHxy AnonHxy added this to the 2.12.0 milestone Sep 13, 2022
@AnonHxy
Copy link
Contributor

AnonHxy commented Sep 13, 2022

@Jason918 PTAL

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@AnonHxy AnonHxy merged commit c5cd566 into apache:master Sep 13, 2022
tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Sep 14, 2022
Technoboy- pushed a commit that referenced this pull request Mar 6, 2023
…on (#17374)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs release/2.11.1 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants