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-12291][Client] 'StartMessageId' and 'RollbackDuration' not working in MultiTopicsReader for non-partitioned topics #12308

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

Jason918
Copy link
Contributor

@Jason918 Jason918 commented Oct 9, 2021

Fixes #12291

Motivation

Bug fix. 'StartMessageId' and 'RollbackDuration' is not working in MultiTopicsReader for non-partitioned topics.

Modifications

This fix is quite simple. Just add startMessageId and startMessageRollbackDurationInSec when creating underlying consumer with ConsumerImpl.newConsumerImpl

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • org.apache.pulsar.client.impl.MultiTopicsReaderTest#testMultiNonPartitionedTopicWithStartMessageId
  • org.apache.pulsar.client.impl.MultiTopicsReaderTest#testMultiNonPartitionedTopicWithRollbackDuration

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

    (Please explain why) simple bug fix.

@gaoran10
Copy link
Contributor

gaoran10 commented Oct 10, 2021

Please modify the check if (startMessageId.equals(MessageId.latest)) of the method ConsumerImpl#hasMessageAvailableAsync() to if (MessageId.latest.equals(startMessageId)), if the startMessageId is null in some conditions if possible, the NPE problem will be present again.

@Jason918
Copy link
Contributor Author

Please modify the check if (startMessageId.equals(MessageId.latest)) of the method ConsumerImpl#hasMessageAvailableAsync() to if (MessageId.latest.equals(startMessageId)), if the startMessageId is null in some conditions if possible, the NPE problem will be present again.

Done.

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Oct 13, 2021
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

3 similar comments
@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@315157973 315157973 left a comment

Choose a reason for hiding this comment

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

LGTM

@315157973
Copy link
Contributor

@codelipenghui codelipenghui merged commit cb48152 into apache:master Nov 4, 2021
codelipenghui pushed a commit that referenced this pull request Nov 5, 2021
…king in MultiTopicsReader for non-partitioned topics (#12308)

Fixes #12291

### Motivation

Bug fix. 'StartMessageId' and 'RollbackDuration' is not working in MultiTopicsReader for non-partitioned topics.

### Modifications

This fix is quite simple. Just add `startMessageId` and `startMessageRollbackDurationInSec` when creating underlying consumer with `ConsumerImpl.newConsumerImpl`

(cherry picked from commit cb48152)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Nov 5, 2021
eolivelli pushed a commit that referenced this pull request Nov 9, 2021
…king in MultiTopicsReader for non-partitioned topics (#12308)

Fixes #12291

### Motivation

Bug fix. 'StartMessageId' and 'RollbackDuration' is not working in MultiTopicsReader for non-partitioned topics.

### Modifications

This fix is quite simple. Just add `startMessageId` and `startMessageRollbackDurationInSec` when creating underlying consumer with `ConsumerImpl.newConsumerImpl`

(cherry picked from commit cb48152)
@eolivelli eolivelli removed this from the 2.10.0 milestone Nov 9, 2021
@eolivelli eolivelli added this to the 2.9.0 milestone Nov 9, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
…king in MultiTopicsReader for non-partitioned topics (apache#12308)

Fixes apache#12291

### Motivation

Bug fix. 'StartMessageId' and 'RollbackDuration' is not working in MultiTopicsReader for non-partitioned topics.

### Modifications

This fix is quite simple. Just add `startMessageId` and `startMessageRollbackDurationInSec` when creating underlying consumer with `ConsumerImpl.newConsumerImpl`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-topic reader ignore .startMessageId(MessageId.earliest).
6 participants