refactor(inkless): stop retention and cleanup jobs during broker shutdown [KC-417] - #739
refactor(inkless): stop retention and cleanup jobs during broker shutdown [KC-417]#739jeqo wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Moves diskless retention and cleanup off Kafka’s shared scheduler to provide bounded broker shutdown.
Changes:
- Adds a dedicated stoppable background-job scheduler.
- Caps retention work per cycle and adds saturation metrics/configuration.
- Expands lifecycle, scheduling, and configuration tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/scala/kafka/server/ReplicaManager.scala |
Owns and shuts down the dedicated scheduler. |
storage/inkless/src/main/java/io/aiven/inkless/config/InklessConfig.java |
Adds the per-cycle partition cap. |
storage/inkless/src/main/java/io/aiven/inkless/control_plane/ControlPlane.java |
Clarifies retention API semantics. |
storage/inkless/src/main/java/io/aiven/inkless/delete/FileCleaner.java |
Adds cooperative shutdown and removes sleeps. |
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java |
Adds bounded, stoppable partition processing. |
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcementScheduler.java |
Supports limited ready-partition polling. |
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcerMetrics.java |
Adds cycle-saturation telemetry. |
storage/inkless/src/test/java/io/aiven/inkless/config/InklessConfigTest.java |
Tests the new configuration. |
storage/inkless/src/test/java/io/aiven/inkless/delete/FileCleanerMockedTest.java |
Tests cleaner shutdown behavior. |
storage/inkless/src/test/java/io/aiven/inkless/delete/RetentionEnforcerTest.java |
Tests caps and cooperative stopping. |
storage/inkless/src/test/java/io/aiven/inkless/delete/RetentionEnforcementSchedulerTest.java |
Tests limited polling and preserved lag. |
docs/inkless/configs.rst |
Documents the new configuration. |
docs/inkless/metrics.rst |
Documents the saturation metric. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c1713c8 to
bf1bc8a
Compare
bf1bc8a to
3ecebea
Compare
5aa0aac to
ecde2df
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java:53
- This lifecycle guarantee contradicts
RetentionEnforcementScheduler.java:115-118, wheregetReadyPartitionsimmediately reschedules every returned partition. Ifclose()stops this loop after the first request, the remaining fetched partitions don't keep their past-due queue slots. Describe the restart-based recovery instead, or requeue the unprocessed entries.
* any later {@code run()} is a no-op. Enforcement is idempotent and per-partition independent, so an
* abandoned cycle loses no durable progress: the partitions it did not reach keep their queue slot and are
* enforced on a later cycle (here or on another broker).
ecde2df to
abfac4c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java:53
getReadyPartitionsreschedules every returned partition before this loop starts, so partitions skipped afterclose()do not keep their queue slot; they move to a new enforcement time. Update this lifecycle contract to state that the work remains recoverable and is re-derived after restart rather than promising that the original slot remains due.
* any later {@code run()} is a no-op. Enforcement is idempotent and per-partition independent, so an
* abandoned cycle loses no durable progress: the partitions it did not reach keep their queue slot and are
* enforced on a later cycle (here or on another broker).
storage/inkless/src/main/java/io/aiven/inkless/config/InklessConfig.java:265
- The dedicated scheduler uses a 500 ms fixed delay after each run, so a sustained backlog is capped at
maxPartitionsPerCycle / (cycle duration + 500 ms). Leaving excess partitions overdue avoids waiting a full enforcement interval, but it does not guarantee that throughput is unchanged from an unbounded cycle. Reword this claim and regeneratedocs/inkless/configs.rst.
+ "cycle; partitions left over stay due and are picked up by the next cycle (500ms later) rather than "
+ "waiting a full enforcement interval, so throughput is not lost. Watch "
storage/inkless/src/main/java/io/aiven/inkless/delete/RetentionEnforcer.java:148
- A sustained backlog reaches this branch after every cycle, and the scheduler can run it every 500 ms. This emits up to two INFO records per second per broker for a condition already exposed by
RetentionEnforcementCycleSaturatedRate, creating avoidable operational log volume. Rate-limit this message or lower it to DEBUG.
LOGGER.info("Retention enforcement cycle saturated at {} partitions; overdue partitions may remain queued",
maxPartitionsPerCycle);
abfac4c to
b5a369c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
core/src/main/scala/kafka/server/ReplicaManager.scala:3429
- The broker-level regression remains untested: the component tests cover each job's stop flag, but they don't exercise
ReplicaManagerowning the executor or prove that shutdown waits for an in-flight job before closinginklessSharedState. A later reorder of this call can restore the original shutdown failure while all added tests still pass. Add aReplicaManagerInklessTestthat blocks a cleaner/enforcer cycle, starts shutdown, and verifies shared state stays open until the job reaches its cooperative stop point.
shutdownInklessBackgroundJobs()
b5a369c to
c9e1a69
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
core/src/test/scala/unit/kafka/server/ReplicaManagerInklessTest.scala:7701
TIMED_WAITINGdoesn't identifybgScheduler.awaitTermination; any earlier shutdown step that uses a timed park can satisfy this predicate. The test would then release the job before shutdown reaches the ordering under test, so movingsharedState.close()ahead of the background-job join could pass. Wait for a stack containing bothshutdownInklessBackgroundJobsandawaitTermination, or expose a latch at that join point.
// Poll for the shutdown thread parking in bgScheduler.awaitTermination rather than sleeping a
// fixed duration: this is the exact moment shutdownInklessBackgroundJobs() is waiting on the
// blocked job, which is what this test needs to observe.
waitUntilTrue(() => shutdownThread.getState == Thread.State.TIMED_WAITING,
"shutdown thread never parked waiting for the in-flight background job")
…down `RetentionEnforcer` and `FileCleaner` ran on the shared `KafkaScheduler`. That scheduler drains its queue on shutdown without interrupting, waiting up to a day, and `BrokerServer.shutdown` awaits it early. An in-flight cycle could therefore block a broker restart. Run both jobs on a dedicated 2-thread daemon scheduler that `ReplicaManager` owns. Shutdown stops new cycles, closes both jobs, and waits 10s before using `shutdownNow()` as a backstop. Shared state stays open during the grace period. Each job stops at a safe boundary. `RetentionEnforcer` checks its stop signal between partitions, issuing one `enforceRetention` call per partition. The control plane already uses one transaction per partition, so this adds no transactions. `FileCleaner` checks its stop signal after fetching work and before deleting from object storage. Both jobs are idempotent and re-derive unfinished work on the next cycle. Remove `FileCleaner`'s in-task no-work and error sleeps. The periodic schedule now owns retry cadence, so an idle or failed cycle does not hold the scheduler thread during shutdown. Add a `ReplicaManagerInklessTest` covering the ordering itself: it blocks a file-cleaner cycle mid-flight, starts shutdown on another thread, and asserts the shared state stays open until the job reaches its stop point. The `FileCleaner`/`RetentionEnforcer` component tests only cover each job's own stop flag; they exercise neither `ReplicaManager` owning the scheduler nor the shutdown order, so they pass unchanged even if a later change reorders `shutdownInklessBackgroundJobs()` back before closing the shared state.
c9e1a69 to
8a333cd
Compare
A broker restart could hang on an in-flight diskless retention or file-cleanup cycle. Both jobs ran on the shared
KafkaScheduler, which drains its queue on shutdown without interrupting, waiting up to a day, andBrokerServer.shutdownawaits it early.This moves both jobs onto a scheduler that
ReplicaManagerowns and makes each job stop at a safe point when closed. Both jobs are idempotent and re-derive unfinished work on the next cycle, so stopping loses no durable progress.Changes
RetentionEnforcerandFileCleaneron a dedicated 2-thread daemon scheduler while preserving their fixed-rate cadence.shutdownNow()as a backstop. The control plane and storage backend stay open during the grace period.enforceRetentioncall per partition; the control plane already uses one transaction per partition, so this adds no transactions.FileCleaner's in-task no-work and error sleeps. The periodic schedule now owns retry cadence.Operator impact
FileCleanerretries an error on its next scheduled cycle rather than after an in-task backoff sleep.Testing
RetentionEnforcerTest: one control-plane call per enforceable partition, stop at a partition boundary, andrun()afterclose()as a no-op.FileCleanerMockedTest: no on-thread sleep on no-work and error paths, deletion skipped whenclose()lands between fetch and delete, andrun()afterclose()as a no-op.ReplicaManagerInklessTest: blocks aFileCleanercycle mid-flight, starts shutdown on another thread, and asserts the shared state stays open until the job reaches its stop point. This exercisesReplicaManagerowning the scheduler and the shutdown ordering itself, which the component tests above do not: they only cover each job's own stop flag, so they would pass unchanged even ifshutdownInklessBackgroundJobsmoved back afterinklessSharedState.close().