Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
BewareMyPower merged PR #25119: URL: https://github.com/apache/pulsar/pull/25119 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
codecov-commenter commented on PR #25119: URL: https://github.com/apache/pulsar/pull/25119#issuecomment-3705591182 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/25119?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 73.85%. Comparing base ([`ff0d0eb`](https://app.codecov.io/gh/apache/pulsar/commit/ff0d0eb2ba97d96c8bc760a1dfe8255efa71d5c1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dbc9a0b`](https://app.codecov.io/gh/apache/pulsar/commit/dbc9a0bc578d9b54d430447d2a770d3c5e3f049b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 8 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/25119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) ```diff @@ Coverage Diff @@ ## master #25119 +/- ## - Coverage 74.82% 73.85% -0.97% + Complexity33836 2319 -31517 Files 1899 1899 Lines149656 149686 +30 Branches 1739317404 +11 - Hits 111979 110557-1422 - Misses2889230242+1350 - Partials 8785 8887 +102 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/25119/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/25119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.56% <100.00%> (-0.33%)` | :arrow_down: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/25119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.99% <20.00%> (-0.22%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/25119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.35% <100.00%> (-1.00%)` | :arrow_down: | 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=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/25119?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/25119?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedCursorImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | `79.03% <100.00%> (-0.85%)` | :arrow_down: | | [...e/pulsar/compaction/AbstractTwoPhaseCompactor.java](https://app.codecov.io/gh/apache/pulsar/pull/25119?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fcompaction%2FAbstractTwoPhaseCompactor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbXBhY3Rpb24vQWJzdHJhY3RUd29QaGFzZUNvbXBhY3Rvci5qYXZh) | `78.87% <100.00%> (+0.20%)` | :arrow_up: | ... and [153 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/25119/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detec
Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
lhotari closed pull request #25119: [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted URL: https://github.com/apache/pulsar/pull/25119 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
BewareMyPower commented on code in PR #25119:
URL: https://github.com/apache/pulsar/pull/25119#discussion_r2656987692
##
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##
@@ -1593,7 +1598,6 @@ public void operationFailed(ManagedLedgerException
exception) {
persistentMarkDeletePosition = null;
inProgressMarkDeletePersistPosition = null;
-lastMarkDeleteEntry = new MarkDeleteEntry(newMarkDeletePosition,
getProperties(), null, null);
Review Comment:
`lastMarkDeleteEntry` will be updated after `internalAsyncMarkDelete` is
done in `alignAcknowledgeStatusAfterPersisted`:
https://github.com/apache/pulsar/blob/ff0d0eb2ba97d96c8bc760a1dfe8255efa71d5c1/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1534
Modifying it before the start of `internalAsyncMarkDelete` is duplicated and
does not make sense.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
lhotari commented on code in PR #25119:
URL: https://github.com/apache/pulsar/pull/25119#discussion_r2655044284
##
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##
@@ -1593,7 +1598,6 @@ public void operationFailed(ManagedLedgerException
exception) {
persistentMarkDeletePosition = null;
inProgressMarkDeletePersistPosition = null;
-lastMarkDeleteEntry = new MarkDeleteEntry(newMarkDeletePosition,
getProperties(), null, null);
Review Comment:
is there a reason for removing this for all cursors?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [fix][broker] Fix compaction horizon might be reset to an old position when phase two is interrupted [pulsar]
Copilot commented on code in PR #25119:
URL: https://github.com/apache/pulsar/pull/25119#discussion_r2651179375
##
pulsar-broker/src/test/java/org/apache/pulsar/compaction/CompactionTest.java:
##
@@ -149,6 +157,11 @@ public void cleanup() throws Exception {
}
}
+@BeforeMethod(alwaysRun = true)
+public void beforeMethod() throws Exception {
+admin.namespaces().removeRetention("my-tenant/my-ns");
Review Comment:
The static injection point should be reset in the existing @BeforeMethod to
ensure test isolation. Add
"AbstractTwoPhaseCompactor.injectionAfterSeekInPhaseTwo = () -> {};" to the
beforeMethod() to prevent injection from one test affecting another test,
especially in case of test failures or when tests run in parallel.
```suggestion
admin.namespaces().removeRetention("my-tenant/my-ns");
AbstractTwoPhaseCompactor.injectionAfterSeekInPhaseTwo = () -> {};
```
##
pulsar-broker/src/main/java/org/apache/pulsar/compaction/AbstractTwoPhaseCompactor.java:
##
@@ -59,6 +59,7 @@
*/
public abstract class AbstractTwoPhaseCompactor extends Compactor {
+ public static volatile Runnable injectionAfterSeekInPhaseTwo = () -> {};
Review Comment:
The static injection field should not be public as it's only intended for
testing purposes. Consider making it package-private or annotating it with
@VisibleForTesting to make its purpose clear and restrict access.
```suggestion
static volatile Runnable injectionAfterSeekInPhaseTwo = () -> {};
```
##
pulsar-broker/src/main/java/org/apache/pulsar/compaction/AbstractTwoPhaseCompactor.java:
##
@@ -59,6 +59,7 @@
*/
public abstract class AbstractTwoPhaseCompactor extends Compactor {
+ public static volatile Runnable injectionAfterSeekInPhaseTwo = () -> {};
Review Comment:
Using a static volatile field for test injection creates potential race
conditions when tests run in parallel. If multiple test instances execute
concurrently, they will interfere with each other's injection callbacks.
Consider using a thread-local or instance-level injection mechanism instead, or
ensure tests using this field cannot run in parallel.
```suggestion
public static Runnable injectionAfterSeekInPhaseTwo = () -> {};
```
##
pulsar-broker/src/test/java/org/apache/pulsar/compaction/CompactionTest.java:
##
@@ -2460,4 +2479,70 @@ public void testEarliestSubsAfterRollover() throws
Exception {
assertEquals(results, expected);
}
+
+@Test
+public void testPhaseTwoInterruption() throws Exception {
+admin.namespaces().setRetention("my-tenant/my-ns", new
RetentionPolicies(60/* 1 hour */, 1024/* 1MB */));
Review Comment:
The comment incorrectly states "1 hour" but the retention value is 60
minutes, which is indeed 1 hour. However, the RetentionPolicies constructor
expects retention time in minutes, not hours. The comment should clarify "60
minutes" to avoid confusion, or if the intent was 1 hour it should be clear
that the first parameter is in minutes.
```suggestion
admin.namespaces().setRetention("my-tenant/my-ns",
new RetentionPolicies(60/* 60 minutes (1 hour) */, 1024/*
1MB */));
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
