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<T> 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<T> 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]
