You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/03/31 06:37:32 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #10095: [Broker] Fix NonDurableCursorImpl initialPosition by startCursorPosition greater than lastConfirmedEntry problem.
congbobo184 opened a new pull request #10095:
URL: https://github.com/apache/pulsar/pull/10095
## Motivation
In order to fix reader set ```startMessageId``` greater than ```lastConfirmedEntry``` probelm.
When the reader set ```startMessageId``` greater than ```lastConfirmedEntry```, generate ```NonDurableCursorImpl``` will carry a ```MarkdeletePosition``` greater than lastConfirmedEntry.
When use ```getNumberOfEntriesInBacklog``` will throw
## implement
When ```startMessageId``` greater than ```lastConfirmedEntry``` use ```lastConfirmedEntry``` as ```markdeletePosition```
### Verifying this change
Add the tests for it
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): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a change in pull request #10095: [Broker] Fix NonDurableCursorImpl initialPosition by startCursorPosition greater than lastConfirmedEntry problem.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #10095:
URL: https://github.com/apache/pulsar/pull/10095#discussion_r604668365
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/NonDurableCursorImpl.java
##########
@@ -40,7 +40,8 @@
// Compare with "latest" position marker by using only the ledger id. Since the C++ client is using 48bits to
// store the entryId, it's not able to pass a Long.max() as entryId. In this case there's no point to require
// both ledgerId and entryId to be Long.max()
- if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()) {
+ if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()
+ || startCursorPosition.compareTo(ledger.lastConfirmedEntry) > 0) {
Review comment:
It seems that the condition `startCursorPosition.compareTo(ledger.lastConfirmedEntry) > 0` already cover the condition `startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()`, so compare with `ledger.lastConfirmedEntry` is enough?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap merged pull request #10095: [Broker] Fix NonDurableCursorImpl initialPosition by startCursorPosition greater than lastConfirmedEntry problem.
Posted by GitBox <gi...@apache.org>.
zymap merged pull request #10095:
URL: https://github.com/apache/pulsar/pull/10095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #10095: [Broker] Fix NonDurableCursorImpl initialPosition by startCursorPosition greater than lastConfirmedEntry problem.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10095:
URL: https://github.com/apache/pulsar/pull/10095#discussion_r604681568
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/NonDurableCursorImpl.java
##########
@@ -40,7 +40,8 @@
// Compare with "latest" position marker by using only the ledger id. Since the C++ client is using 48bits to
// store the entryId, it's not able to pass a Long.max() as entryId. In this case there's no point to require
// both ledgerId and entryId to be Long.max()
- if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()) {
+ if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()
+ || startCursorPosition.compareTo(ledger.lastConfirmedEntry) > 0) {
Review comment:
good suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] klwilson227 commented on a change in pull request #10095: [Broker] Fix NonDurableCursorImpl initialPosition by startCursorPosition greater than lastConfirmedEntry problem.
Posted by GitBox <gi...@apache.org>.
klwilson227 commented on a change in pull request #10095:
URL: https://github.com/apache/pulsar/pull/10095#discussion_r605014383
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/NonDurableCursorImpl.java
##########
@@ -40,7 +40,8 @@
// Compare with "latest" position marker by using only the ledger id. Since the C++ client is using 48bits to
// store the entryId, it's not able to pass a Long.max() as entryId. In this case there's no point to require
// both ledgerId and entryId to be Long.max()
- if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()) {
+ if (startCursorPosition == null || startCursorPosition.getLedgerId() == PositionImpl.latest.getLedgerId()
+ || startCursorPosition.compareTo(ledger.lastConfirmedEntry) > 0) {
Review comment:
Do we have test case coverage?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org