6818: Gracefully stop compaction tasks by ca61688 · Pull Request #6913 · gchq/sleeper

ca61688 · 2026-03-26T12:15:35Z

Make sure you have checked all steps below.

Issue

My PR fully resolves the following issues. I've referenced an issue in the PR title, for example "Issue 1234 - My
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
- Resolves Gracefully stop compaction tasks after a maximum age #6818

Tests

My PR adds the following tests based on our test strategy OR does not need testing for this extremely good reason:
-CompactionTaskTerminateTest - StopAfterMaxAliveTime

Documentation

In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
separate issue for that below.
If I have added new Java code, I have added Javadoc that explains it following our conventions and style.
If I have added or removed any dependencies from the project, I have updated the NOTICES file.

patchwork01

I think there's a problem we didn't think of here. If we add this as it is, the tasks will shut down at very similar times if they started at similar times. We'll want to avoid a situation where too many compaction tasks shut down at once. For that reason I don't think we can merge this as it is.

One option would be to randomise the lifetime of the task when it starts up. We could have both a maximum lifetime and an amount of time to jitter it by. For example, if the maximum lifetime is 24 hours, and the jitter is 2 hours, and you have 100 tasks starting at once, then they'd stop gradually over a period of 2 hours instead of all at once after 24 hours.

For how to control the jitter in tests, you could look at ExponentialBackoffWithJitterTest, and how we've used ExponentialBackoffWithJitterTestHelper.

...ction/compaction-core/src/test/java/sleeper/compaction/core/task/CompactionTaskTestBase.java

java/core/src/main/java/sleeper/core/properties/instance/CompactionProperty.java

ca61688 · 2026-03-31T14:53:47Z

Comments addressed, back in review

patchwork01 · 2026-04-02T08:22:48Z

java/core/src/main/java/sleeper/core/properties/instance/CompactionProperty.java

+            .description("The max ammount of jitter to add to a new compaction task's keep alive time.\n" +
+                    "A random number of minutes between 0 and this value will be added to the keep alive time above " +
+                    "for each new compaction task.\n" +
+                    "This lowers the chance that all the compaction tasks stop at the same time.")


There's a typo at "ammount".

It looks like there are some words missing at "keep alive time above for each new compaction task".

patchwork01 · 2026-04-02T08:25:41Z

java/core/src/main/java/sleeper/core/properties/instance/CompactionProperty.java

+    UserDefinedInstanceProperty COMPACTION_TASK_KEEP_ALIVE_TIME_IN_MINMUTES = Index.propertyBuilder("sleeper.compaction.task.keep.alive.time.minutes")
+            .description("The total time in minutes that a compaction task can be alive for before it is terminated.\n" +
+                    "When the task has finished it's current job, it will calculate how long it's been running and " +
+                    "if it's been running longer than the max alive time it will terminate.\n" +
+                    "A new task will then begin running to process remaining compaction jobs.\n" +
+                    "This allows the user to gracefully upgrade Sleeper to a newer version.")


There's some disagreement here between "max alive time" and "keep alive time". I think the idea that this is a keep alive is a little misleading because we don't do anything to keep it alive, we stop it when it would otherwise carry on. I think "max alive time" makes more sense, but it seems inaccurate when the jitter is added on top. I'd consider making the jitter subtract from the max alive time rather than add to it.

This naming is also used in the tests, including the new comments and the nested test class.

patchwork01 · 2026-04-02T08:27:27Z

java/core/src/test/java/sleeper/core/testutils/TestSupplier.java

+ * Wrapper class around the Supplier Instant interface to be able to assert cleanly there are no remaining expected
+ * time calls.
+ */
+public class TestSupplier implements Supplier<Instant> {


The name TestSupplier doesn't say what it's supplying. TestInstantSupplier seems better?

patchwork01 · 2026-04-02T11:45:32Z

java/compaction/compaction-core/src/main/java/sleeper/compaction/core/task/CompactionTask.java

    private final Supplier<String> jobRunIdSupplier;
    private final Supplier<Instant> timeSupplier;
    private final ThreadSleep threadSleep;
+    private final Long maxAliveTime;


This could do with stating the units, i.e. minutes rather than time.

It doesn't look like there's a reason to box this value? It can just be a primitive long?

patchwork01 · 2026-04-02T11:49:47Z

.../compaction-core/src/test/java/sleeper/compaction/core/task/CompactionTaskTerminateTest.java

+        private CompactionTask generateNewCompactionTask(String taskId) {
+            return new CompactionTask(instanceProperties, null,
+                    null, null,
+                    null, null,
+                    null, null,
+                    null, null, taskId);


Rather than passing in lots of nulls, we could extract the code that calls the constructor in the test base class, and reuse that?

I don't think you'll need this method if you add tests for the jitter behaviour though.

patchwork01 · 2026-04-02T11:55:28Z

.../compaction-core/src/test/java/sleeper/compaction/core/task/CompactionTaskTerminateTest.java

+        @Test
+        void shouldGenerateUniqueMaxAliveTimesForUniqueTasks() {
+            //Given
+            instanceProperties.setNumber(COMPACTION_TASK_KEEP_ALIVE_JITTER_IN_MINUTES, Integer.MAX_VALUE);
+            CompactionTask task1 = generateNewCompactionTask("task1");
+            CompactionTask task2 = generateNewCompactionTask("task2");
+
+            //When/Then
+            assertThat(task1.getMaxAliveTime()).isNotEqualTo(task2.getMaxAliveTime());


This isn't really testing the jitter behaviour. In ExponentialBackoffWithJitter, we passed in a constructor parameter DoubleSupplier randomJitterFraction, that's usually implemented as Math::random. With that, you can replace the Math.random call with your own code, to take control over how much jitter happens in a test. Take a look at ExponentialBackoffWithJitterTest.

That way you won't need to add the getMaxAliveTime method, and you can provide real test coverage of the behaviour, e.g. if the minimum amount of jitter happens, then it'll stop at the first possible time, if the maximum amount of jitter happens it'll only stop after the last possible time.

patchwork01 · 2026-04-02T12:00:53Z

java/compaction/compaction-core/src/main/java/sleeper/compaction/core/task/CompactionTask.java

+                + (long) (Math.random() * instanceProperties.getLong(COMPACTION_TASK_KEEP_ALIVE_JITTER_IN_MINUTES));
+    }
+
+    public Long getMaxAliveTime() {


It doesn't look like this getter should be necessary. I'm not sure the testing against this is really adding much.

I think either it's worth testing the behaviour and we should wire in a test fake for Math.random, or it's not and we just test that it'll terminate using times where it should terminate even with the maximum jitter.

6818: Gracefully stop compaction tasks

ad2a77f

ca61688 linked an issue Mar 26, 2026 that may be closed by this pull request

Gracefully stop compaction tasks after a maximum age #6818

Open

ca61688 added 14 commits March 26, 2026 16:17

Merge branch 'develop' into 6818-gracefully-stop-compaction-tasks

fc9c98c

6818: Stop infinite while loop

755a37f

6818: Revert changes to test

696f324

6818: Fix test

9a7fda8

6818: Fix test

a2947e2

6818: Fix test

8eb7832

6818: Fix test

b8ebe86

Merge branch 'develop' into 6818-gracefully-stop-compaction-tasks

604afda

6818: Add keep alive checks to compaction tests

d95e9fc

6818: Add keep alive checks to compaction tests

2bb8a14

Merge branch 'develop' into 6818-gracefully-stop-compaction-tasks

3b03a5c

6818: Add keep alive checks to compaction tests

34a3ea3

6818: Improve compaction max alive time description

2b3ef99

6818: Fix comment in test

f067d8d

ca61688 marked this pull request as ready for review March 31, 2026 09:12

ca61688 added the needs-reviewer Pull requests that need a reviewer to be assigned label Mar 31, 2026

patchwork01 requested changes Mar 31, 2026

View reviewed changes

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Mar 31, 2026

patchwork01 assigned ca61688 Mar 31, 2026

ca61688 added 7 commits March 31, 2026 12:45

6818: Remove duplicate test method

01b2497

6818: Create wrapper around test supplier for better assertions

3b5edff

6818: Create wrapper around test supplier for better assertions

7afa89a

6818: Switch keep compaction task align property to minuts from seconds

2096305

6818: Correct max alive time property description

7fc8cf4

6818: Add random jitter to compaction task max alive time

3c2cc78

6818: Add random jitter to compaction task max alive time

147369f

ca61688 removed their assignment Mar 31, 2026

ca61688 added the needs-reviewer Pull requests that need a reviewer to be assigned label Mar 31, 2026

ca61688 added 2 commits March 31, 2026 15:15

6818: Improve compaction max alive jitter description

59cdb69

Merge branch 'develop' into 6818-gracefully-stop-compaction-tasks

405fd5d

rtjd6554 approved these changes Apr 2, 2026

View reviewed changes

patchwork01 reviewed Apr 2, 2026

View reviewed changes

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Apr 2, 2026

patchwork01 assigned ca61688 Apr 2, 2026

patchwork01 reviewed Apr 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6818: Gracefully stop compaction tasks#6913

6818: Gracefully stop compaction tasks#6913
ca61688 wants to merge 24 commits intodevelopfrom
6818-gracefully-stop-compaction-tasks

ca61688 commented Mar 26, 2026

Uh oh!

patchwork01 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ca61688 commented Mar 31, 2026

Uh oh!

patchwork01 Apr 2, 2026

Uh oh!

patchwork01 Apr 2, 2026 •

edited

Loading

Uh oh!

patchwork01 Apr 2, 2026

Uh oh!

patchwork01 Apr 2, 2026

Uh oh!

patchwork01 Apr 2, 2026 •

edited

Loading

Uh oh!

patchwork01 Apr 2, 2026 •

edited

Loading

Uh oh!

patchwork01 Apr 2, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ca61688 commented Mar 26, 2026

Issue

Tests

Documentation

Uh oh!

patchwork01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ca61688 commented Mar 31, 2026

Uh oh!

patchwork01 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patchwork01 Apr 2, 2026 •

edited

Loading

patchwork01 Apr 2, 2026 •

edited

Loading

patchwork01 Apr 2, 2026 •

edited

Loading

patchwork01 Apr 2, 2026 •

edited

Loading