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 2022/04/05 14:52:13 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
lhotari opened a new pull request, #15031:
URL: https://github.com/apache/pulsar/pull/15031
### Motivation
- missed updates to `lastMarkDeleteEntry` can lead to the subscription and consuming getting stuck. This seems to impact both Key_shared and Shared subscriptions.
### Modifications
- when updating `lastMarkDeleteEntry` field, make sure that the new value is later than the previous value.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842889574
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1796,7 +1796,14 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
// ledger is postponed to when the counter goes to 0.
PENDING_MARK_DELETED_SUBMITTED_COUNT_UPDATER.incrementAndGet(this);
- lastMarkDeleteEntry = mdEntry;
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+ // keep the current value since it's later then the mdEntry.newPosition
Review Comment:
then we can add DEBUG in the other branch, the "new" code that you are adding
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842887286
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
there would be a lot of logging since it's very common that it would happen.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
Do you think it's useful to log it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842887686
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1796,7 +1796,14 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
// ledger is postponed to when the counter goes to 0.
PENDING_MARK_DELETED_SUBMITTED_COUNT_UPDATER.incrementAndGet(this);
- lastMarkDeleteEntry = mdEntry;
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+ // keep the current value since it's later then the mdEntry.newPosition
Review Comment:
there would be a lot of logging since it's very common that it would happen.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842897048
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1796,7 +1796,14 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
// ledger is postponed to when the counter goes to 0.
PENDING_MARK_DELETED_SUBMITTED_COUNT_UPDATER.incrementAndGet(this);
- lastMarkDeleteEntry = mdEntry;
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+ // keep the current value since it's later then the mdEntry.newPosition
Review Comment:
or is this better
```java
if (log.isDebugEnabled()) {
log.debug("Ignoring update {} to lastMarkDeleteEntry {} since current value is later", mdEntry, last);
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#issuecomment-1091950714
I made a follow-up PR #15067 to improve the solution. Please review.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#issuecomment-1106000020
Why does this patch not add a test ?
@eolivelli @lhotari
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842895246
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1796,7 +1796,14 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
// ledger is postponed to when the counter goes to 0.
PENDING_MARK_DELETED_SUBMITTED_COUNT_UPDATER.incrementAndGet(this);
- lastMarkDeleteEntry = mdEntry;
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+ // keep the current value since it's later then the mdEntry.newPosition
Review Comment:
Do you mean something like this?
```java
if (log.isDebugEnabled()) {
log.debug("Keeping {} since it's newer than {}", last, mdEntry);
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#issuecomment-1088813700
@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#issuecomment-1088814581
@lhotari:Thanks for providing doc info!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842888222
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
Do you think it's useful to log it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842886006
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1796,7 +1796,14 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
// ledger is postponed to when the counter goes to 0.
PENDING_MARK_DELETED_SUBMITTED_COUNT_UPDATER.incrementAndGet(this);
- lastMarkDeleteEntry = mdEntry;
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+ // keep the current value since it's later then the mdEntry.newPosition
Review Comment:
what about adding a DEBUG statement here ?
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
what about adding a DEBUG statement here ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #15031:
URL: https://github.com/apache/pulsar/pull/15031
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842887286
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
there would be a lot of logging since it's very common that it would happen.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#discussion_r842898025
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2097,6 +2102,22 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
}
}
+ // update lastMarkDeleteEntry field if newPosition is later than the current lastMarkDeleteEntry.newPosition
+ private void updateLastMarkDeleteEntryToLatest(final PositionImpl newPosition,
+ final Map<String, Long> properties) {
+ LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+ if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+ // keep current value, don't update
Review Comment:
let's address logging in the previous case and use a similar resolution here
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15031: [ML] Fix race condition in updating lastMarkDeleteEntry field
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15031:
URL: https://github.com/apache/pulsar/pull/15031#issuecomment-1088821940
btw. The problem scenario looked like very similar as in #14261. However there was a difference.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org