Skip to content

[ML] Fix race condition in updating lastMarkDeleteEntry field #15031

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.

Sign up for GitHub

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

Merged
eolivelli merged 1 commit into from
Apr 5, 2022
Merged

[ML] Fix race condition in updating lastMarkDeleteEntry field #15031

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,7 +1742,7 @@ public void asyncMarkDelete(final Position position, Map<String, Long> propertie
// Apply rate limiting to mark-delete operations
if (markDeleteLimiter != null && !markDeleteLimiter.tryAcquire()) {
isDirty = true;
lastMarkDeleteEntry = new MarkDeleteEntry(newPosition, properties, null, null);
updateLastMarkDeleteEntryToLatest(newPosition, properties);
callback.markDeleteComplete(ctx);
return;
}
Expand Down Expand Up @@ -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

@eolivelli eolivelli Apr 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about adding a DEBUG statement here ?

@lhotari lhotari Apr 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

there would be a lot of logging since it's very common that it would happen.

@eolivelli eolivelli Apr 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

then we can add DEBUG in the other branch, the "new" code that you are adding

@lhotari lhotari Apr 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean something like this?

                if (log.isDebugEnabled()) {
                    log.debug("Keeping {} since it's newer than {}", last, mdEntry);
                }

@lhotari lhotari Apr 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

or is this better

                if (log.isDebugEnabled()) {
                    log.debug("Ignoring update {} to lastMarkDeleteEntry {} since current value is later", mdEntry, last);
                }

return last;
} else {
return mdEntry;
}
});

persistPositionToLedger(cursorLedger, mdEntry, new VoidCallback() {
@Override
Expand Down Expand Up @@ -2063,9 +2070,7 @@ public void asyncDelete(Iterable<Position> positions, AsyncCallbacks.DeleteCallb
// Apply rate limiting to mark-delete operations
if (markDeleteLimiter != null && !markDeleteLimiter.tryAcquire()) {
isDirty = true;
PositionImpl finalNewMarkDeletePosition = newMarkDeletePosition;
LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this,
last -> new MarkDeleteEntry(finalNewMarkDeletePosition, last.properties, null, null));
updateLastMarkDeleteEntryToLatest(newMarkDeletePosition, null);
callback.deleteComplete(ctx);
return;
}
Expand Down Expand Up @@ -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

@eolivelli eolivelli Apr 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about adding a DEBUG statement here ?

@lhotari lhotari Apr 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

let's address logging in the previous case and use a similar resolution here

return last;
} else {
// use given properties or when missing, use the properties from the previous field value
Map<String, Long> propertiesToUse =
properties != null ? properties : (last != null ? last.properties : Collections.emptyMap());
return new MarkDeleteEntry(newPosition, propertiesToUse, null, null);
}
});
}

/**
* Given a list of entries, filter out the entries that have already been individually deleted.
*
Expand Down