Skip to content

Comments

[improve][transaction] Add logs for REST API in transaction#15081

Merged
gaozhangmin merged 1 commit intoapache:masterfrom
gaozhangmin:fix-error-exception-handle
Apr 13, 2022
Merged

[improve][transaction] Add logs for REST API in transaction#15081
gaozhangmin merged 1 commit intoapache:masterfrom
gaozhangmin:fix-error-exception-handle

Conversation

@gaozhangmin
Copy link
Contributor

@gaozhangmin gaozhangmin commented Apr 8, 2022

Motivation

Add logs when broker occur error.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • no-need-doc

code optimize. no new feature added.

@gaozhangmin gaozhangmin changed the title commit comments [improve][transaction] Solve rest api exceptions don't throw on broker server side Apr 8, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 8, 2022
@mattisonchao
Copy link
Member

Looks like you just add the error log here, a little bit confused about your PR title and description.

@gaozhangmin
Copy link
Contributor Author

Looks like you just add the error log here, a little bit confused about your PR title and description.

Without this, broker server side logged nothing as issue #14851.

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

This PR doesn't seem to fix the #14851 . I think we need to find the root cause of this issue.

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM

@RobertIndie RobertIndie added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/transaction labels Apr 8, 2022
@RobertIndie RobertIndie added this to the 2.11.0 milestone Apr 8, 2022
@Technoboy- Technoboy- changed the title [improve][transaction] Solve rest api exceptions don't throw on broker server side [improve][transaction] Add logs for REST API in transaction Apr 8, 2022
@Technoboy-
Copy link
Contributor

This PR doesn't seem to fix the #14851 . I think we need to find the root cause of this issue.

Only add logs when error. It's not related to #14851.

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

This PR doesn't seem to fix the #14851 . I think we need to find the root cause of this issue.

Only add logs when error. It's not related to #14851.

Get it. Thanks for your explanation. @Technoboy-

@gaozhangmin
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

Looks like we will return 307, and this redirect exception may don't need to log error.

@ApiResponse(code = 307, message = "Topic is not owned by this broker!"),

@gaozhangmin
Copy link
Contributor Author

Looks like we will return 307, and this redirect exception may don't need to log error.

@ApiResponse(code = 307, message = "Topic is not owned by this broker!"),

@mattisonchao redirection exception is thrown in method validateTopicName. This error won't be caught by CompletableFuture exceptionally

@mattisonchao
Copy link
Member

mattisonchao commented Apr 8, 2022

@gaozhangmin
Would you mind checking this code, looks like this method will invoke validateTopicOwnershipAsync?

protected CompletableFuture<TransactionPendingAckStats> internalGetPendingAckStats(
boolean authoritative, String subName) {
return getExistingPersistentTopicAsync(authoritative)
.thenApply(topic -> topic.getTransactionPendingAckStats(subName));
}

@gaozhangmin
Copy link
Contributor Author

@mattisonchao PTAL again.

@gaozhangmin
Copy link
Contributor Author

/pulsarbot run-failure-checks

@gaozhangmin gaozhangmin force-pushed the fix-error-exception-handle branch from b8867e8 to 7bec9f5 Compare April 11, 2022 08:46
@gaozhangmin
Copy link
Contributor Author

/pulsarbot run-failure-checks

@gaozhangmin gaozhangmin force-pushed the fix-error-exception-handle branch from 7bec9f5 to 8d294c6 Compare April 12, 2022 03:25
@gaozhangmin gaozhangmin force-pushed the fix-error-exception-handle branch from 2bcb013 to f3854b2 Compare April 12, 2022 09:09
@gaozhangmin gaozhangmin merged commit 19120e6 into apache:master Apr 13, 2022
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Apr 18, 2022
Co-authored-by: gavingaozhangmin <gavingaozhangmin@didiglobal.com>
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
Co-authored-by: gavingaozhangmin <gavingaozhangmin@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/transaction doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants