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/10/11 15:47:03 UTC
[GitHub] [pulsar] lordcheng10 opened a new pull request, #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
lordcheng10 opened a new pull request, #18007:
URL: https://github.com/apache/pulsar/pull/18007
### Motivation
In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown：
https://github.com/apache/pulsar/blob/36806853b5a445299d8f9a6ba89905c1915345a3/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1760-L1765
should be modified to:
```
if (newMarkDeletePosition.compareTo(markDeletePosition) <= 0) {
throw new MarkDeletingMarkedPosition(
"Mark deleting an already mark-deleted position. Current mark-delete: " + markDeletePosition
+ " -- attempted mark delete: " + newMarkDeletePosition);
}
```
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277124496
> The change looks good me.
>
> It's better to add a test since this looks like a boundary problem. We should have a test to enforce correct behavior
PTAL,thanks! @codelipenghui @AnonHxy @Jason918 @Technoboy-
add test markDeleteThePositionTwice.
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277472555
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1278680020
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1278920158
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1278667930
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, skip the update of markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277229703
> Does this mean, before this change, user can `ack` the same messageId repeatedly without getting exception. But with this change, user will get an exception?
@Jason918
After the change, the user ack does not need to throw an exception.
When newMarkDeletePosition==markDeletePosition, it should have the same behavior as newMarkDeletePosition<markDeletePosition.
There is just a lack of corresponding equality judgments.
--
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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996649306
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
Changing the 1993 line with the equals method is better because it is weird to use object address to determine.
--
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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996598422
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1906,7 +1906,9 @@ public void asyncMarkDelete(final Position position, Map<String, Long> propertie
lock.writeLock().lock();
try {
- newPosition = setAcknowledgedPosition(newPosition);
+ if (!newPosition.equals(markDeletePosition)) {
+ newPosition = setAcknowledgedPosition(newPosition);
Review Comment:
I don't understand why we are skipping this method, because I found that this judgment already exists in `setAcknowledgedPosition`.
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1279777910
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277494503
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277160546
@aloyszhang PTAL,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 #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1317958218
The pr had no activity for 30 days, mark with Stale label.
--
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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996596014
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
If `current` is equal to `mdEntry.newPosition`, what is the difference between returning `current` and `mdEntry.newPosition`?
The `PositionImpl` already override the equals method.
--
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- closed pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
URL: https://github.com/apache/pulsar/pull/18007
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1278560675
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1275576551
> The change looks good me.
>
> It's better to add a test since this looks like a boundary problem. We should have a test to enforce correct behavior
OK, I will fixed
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1279964217
/pulsarbot run-failure-checks
--
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] congbobo184 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1318549653
@lordcheng10 hi, I move this PR to release/2.9.5, if you have any questions, please ping me. 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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996596014
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
If `current` is equal to `mdEntry.newPosition`, what is the difference between returning `current` and `mdEntry.newPosition`?
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277723195
/pulsarbot run-failure-checks
--
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 #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1543181570
The pr had no activity for 30 days, mark with Stale label.
--
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] michaeljmarshall commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1610271989
As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, skip the update of markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277222862
> > Does this mean, before this change, user can `ack` the same messageId repeatedly without getting exception. But with this change, user will get an exception?
>
> The user will not get any exceptions.
>
> https://github.com/apache/pulsar/blob/c73285205dafaf5bed827ad99f7fb8edd3ddb7f2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L390-L391
>
>
> https://github.com/apache/pulsar/blob/c73285205dafaf5bed827ad99f7fb8edd3ddb7f2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L444-L464
Sorry for not describing clearly, when newMarkDeletePosition is equal to the current markDeletePosition, it should perform the same behavior as less than. @Technoboy-
--
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 #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277206906
> Does this mean, before this change, user can `ack` the same messageId repeatedly without getting exception. But with this change, user will get an exception?
The user will not get any exceptions.
https://github.com/apache/pulsar/blob/c73285205dafaf5bed827ad99f7fb8edd3ddb7f2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L390-L391
https://github.com/apache/pulsar/blob/c73285205dafaf5bed827ad99f7fb8edd3ddb7f2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L444-L464
--
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] lordcheng10 commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996647872
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -974,6 +974,22 @@ void rewind() throws Exception {
assertEquals(c1.getNumberOfEntries(), 2);
}
+ @Test(timeOut = 20000)
+ void markDeleteThePositionTwice() throws Exception {
+ ManagedLedger ledger = factory.open("my_test_ledger", new ManagedLedgerConfig().setMaxEntriesPerLedger(10));
+ ManagedCursor cursor = ledger.openCursor("c1");
+ Position p1 = ledger.addEntry("dummy-entry-1".getBytes(Encoding));
+
+ assertEquals(cursor.getNumberOfEntries(), 1);
+
+ cursor.markDelete(p1);
+ assertFalse(cursor.hasMoreEntries());
+ assertEquals(cursor.getNumberOfEntries(), 0);
+
+ // markDelete p1 again
+ cursor.markDelete(p1);
Review Comment:
OK
--
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] codelipenghui commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996593962
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -974,6 +974,22 @@ void rewind() throws Exception {
assertEquals(c1.getNumberOfEntries(), 2);
}
+ @Test(timeOut = 20000)
+ void markDeleteThePositionTwice() throws Exception {
+ ManagedLedger ledger = factory.open("my_test_ledger", new ManagedLedgerConfig().setMaxEntriesPerLedger(10));
+ ManagedCursor cursor = ledger.openCursor("c1");
+ Position p1 = ledger.addEntry("dummy-entry-1".getBytes(Encoding));
+
+ assertEquals(cursor.getNumberOfEntries(), 1);
+
+ cursor.markDelete(p1);
+ assertFalse(cursor.hasMoreEntries());
+ assertEquals(cursor.getNumberOfEntries(), 0);
+
+ // markDelete p1 again
+ cursor.markDelete(p1);
Review Comment:
Do we need to verify the method `setAcknowledgedPosition` will not invoke?
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1279885193
/pulsarbot run-failure-checks
--
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 #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1356979014
The pr had no activity for 30 days, mark with Stale label.
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277122109
> The change looks good me.
>
> It's better to add a test since this looks like a boundary problem. We should have a test to enforce correct behavior
PTAL,thanks! @codelipenghui
add test markDeleteThePositionTwice.
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277372352
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, skip the update of markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277221472
> Does this mean, before this change, user can `ack` the same messageId repeatedly without getting exception. But with this change, user will get an exception?
Sorry for not describing clearly, when newMarkDeletePosition is equal to the current markDeletePosition, it should perform the same behavior as less than
--
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] codecov-commenter commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277302704
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18007?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@92b4708`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/pulsar/pull/18007?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18007 +/- ##
=========================================
Coverage ? 70.74%
Complexity ? 437
=========================================
Files ? 26
Lines ? 2246
Branches ? 245
=========================================
Hits ? 1589
Misses ? 484
Partials ? 173
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `70.74% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
--
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] lordcheng10 commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996649885
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
OK， I will fix
--
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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996596014
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
If `current` is equal to `mdEntry.newPosition`, what is the difference between returning `current` and `mdEntry.newPosition`?
The `PositionImpl` already override the equals method.
--
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] lordcheng10 commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1278670993
/pulsarbot run-failure-checks
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, when newMarkDeletePosition is equal to markDeletePosition, an exception is thrown
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277123435
> The change looks good me.
>
> It's better to add a test since this looks like a boundary problem. We should have a test to enforce correct behavior
PTAL,thanks! @codelipenghui @AnonHxy
add test markDeleteThePositionTwice.
--
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] mattisonchao commented on a diff in pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#discussion_r996596014
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -1980,7 +1982,7 @@ void internalMarkDelete(final MarkDeleteEntry mdEntry) {
}
PositionImpl inProgressLatest = INPROGRESS_MARKDELETE_PERSIST_POSITION_UPDATER.updateAndGet(this, current -> {
- if (current != null && current.compareTo(mdEntry.newPosition) > 0) {
+ if (current != null && current.compareTo(mdEntry.newPosition) >= 0) {
Review Comment:
If "current" is equal to "mdEntry.newPosition", what is the difference between returning `current` and `mdEntry.newPosition`?
--
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] lordcheng10 commented on pull request #18007: [fix][broker] In the setAcknowledgedPosition method, newMarkDeletePosition should be greater than markDeletePosition
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1277514207
/pulsarbot run-failure-checks
--
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] poorbarcode commented on pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #18007:
URL: https://github.com/apache/pulsar/pull/18007#issuecomment-1501544929
Since we will start the RC version of `3.0.0` on `2023-04-11`, I will change the label/milestone of PR who have not been merged.
- The PR of type `feature` is deferred to `3.1.0`
- The PR of type `fix` is deferred to `3.0.1`
So drag this PR to `3.0.1`
--
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] lordcheng10 closed pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
Posted by "lordcheng10 (via GitHub)" <gi...@apache.org>.
lordcheng10 closed pull request #18007: [fix][broker] Reduce unnecessary persistence of markdelete
URL: https://github.com/apache/pulsar/pull/18007
--
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