-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Added multiple position delete in ManagedLedger #1450
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
Conversation
|
retest this please |
fc2e376 to
6f4024d
Compare
|
|
||
| @Override | ||
| public void delete(final Position position) throws InterruptedException, ManagedLedgerException { | ||
| checkNotNull(position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have these checks in ManagedCursorImpl.java:1524 (asyncDelete)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would fail anyway the cast: PositionImpl position = (PositionImpl) pos;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the checkNotNull?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the cast will fail if pos is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(throwing NPE)
| if (STATE_UPDATER.get(this) == State.Closed) { | ||
| @Override | ||
| public void asyncDelete(Iterable<Position> positions, AsyncCallbacks.DeleteCallback callback, Object ctx) { | ||
| if (state == State.Closed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it as STATE_UPDATER.get(this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no difference with STATE_UPDATER.get(this). Reading a volatile variable gives the last value of it.
| log.debug("[{}] Found a position range to mark delete for cursor {}: {} ", ledger.getName(), | ||
| name, range); | ||
| } | ||
| // Bug:7062188 - markDeletePosition can sometimes be stuck at the beginning of an empty ledger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the address where we can find this bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment was there in the code. If you can find it in the old Yahoo bugzilla... I'll put a link :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the ticket - not very informative.
I would suggest removing the comment altogether unless we have seen the issue again in recent history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not related with this change but will remove
|
@jai1 Updated |
Motivation
To support the grouping of acknowledgements, we need to be able to pass multiple message ids to managed ledger to delete individually.
This change will be used then in broker to do group acknowledgment