Skip to content

Conversation

@Technoboy-
Copy link
Contributor

Motivation

Currently, only seek doesn't support backoff in consumer side, see :

private Optional<CompletableFuture<Void>> seekAsyncCheckState(String seekBy) {
if (getState() == State.Closing || getState() == State.Closed) {
return Optional.of(FutureUtil
.failedFuture(new PulsarClientException.AlreadyClosedException(
String.format("The consumer %s was already closed when seeking the subscription %s of the"
+ " topic %s to %s", consumerName, subscription, topicName.toString(), seekBy))));
}
if (!isConnected()) {
return Optional.of(FutureUtil.failedFuture(new PulsarClientException(
String.format("The client is not connected to the broker when seeking the subscription %s of the "
+ "topic %s to %s", subscription, topicName.toString(), seekBy))));
}
return Optional.empty();
}

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- self-assigned this Aug 9, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Aug 9, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 9, 2023
@Technoboy- Technoboy- closed this Aug 15, 2023
@Technoboy- Technoboy- reopened this Aug 15, 2023
@codelipenghui codelipenghui added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Aug 15, 2023
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test to cover the new changes?

log.error("[{}][{}] Failed to reset subscription: {}", topic, subscription, e.getCause().getMessage());
clearIncomingMessages();
seekFuture.complete(null);
}).exceptionally(e -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the exception is retriable, I think we should also perform a backoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the server side will response with MetadataError/UnknownError, which we can't retry. I see other methods here, also not handle exception to take a retry.

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

❌ Patch coverage is 69.84127% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.91%. Comparing base (2ab184e) to head (1869a42).
⚠️ Report is 1774 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 69.84% 17 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20963       +/-   ##
=============================================
+ Coverage     37.11%   72.91%   +35.80%     
- Complexity    12250    32287    +20037     
=============================================
  Files          1698     1875      +177     
  Lines        129813   141668    +11855     
  Branches      14156    15974     +1818     
=============================================
+ Hits          48179   103301    +55122     
+ Misses        75318    30137    -45181     
- Partials       6316     8230     +1914     
Flag Coverage Δ
inttests 24.74% <41.26%> (+0.35%) ⬆️
systests 25.65% <41.26%> (+0.24%) ⬆️
unittests 72.06% <69.84%> (+39.84%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 76.87% <69.84%> (+20.99%) ⬆️

... and 1434 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Technoboy-
Copy link
Contributor Author

Is it possible to add a test to cover the new changes?

Added test.

@Technoboy- Technoboy- closed this Aug 16, 2023
@Technoboy- Technoboy- reopened this Aug 16, 2023
@Technoboy- Technoboy- closed this Aug 21, 2023
@Technoboy- Technoboy- reopened this Aug 21, 2023
@Technoboy- Technoboy- merged commit ee91edc into apache:master Aug 21, 2023
lhotari pushed a commit that referenced this pull request Mar 27, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
(cherry picked from commit ee91edc)
(cherry picked from commit 9ea7f60)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
(cherry picked from commit ee91edc)
(cherry picked from commit 9ea7f60)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
(cherry picked from commit ee91edc)
(cherry picked from commit 9ea7f60)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
(cherry picked from commit ee91edc)
(cherry picked from commit 9ea7f60)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
(cherry picked from commit ee91edc)
(cherry picked from commit 9ea7f60)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/client cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.4 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.

6 participants