Basekick-Labs / Basekick-Labs/arc

design: should a writer be able to hold the compactor lease when the compactor is gone?

Open
#897 0 comments 0 reactions 0 assignees View on GitHub
design
Dominant language
Go
Stars
677
Forks
53
Avg merge
9h 14m
Merged PRs (30d)
164

Description

Design question, deliberately unsettled. Raised while shipping #876; parked because it is about **resiliency**, not failover, and the answer is a product decision rather than a bug fix.

## The question

When the dedicated compactor node dies, should the compactor lease move to a healthy **writer**, so compaction continues — or should compaction stop until a compactor node exists again?

Today it moves to a writer. That has been the behaviour since #390 ("automatic compactor failover"), and the file has documented it since: *"On failover, a RoleWriter can hold the lease without restarting."* `CompactorFailoverManager.selectNewCompactor` prefers `RoleCompactor` nodes and falls back to healthy `RoleWriter` nodes.

## The position that prompted this

A compactor should be **dedicated**, in Pattern 1 and Pattern 2 alike. Compaction is CPU- and memory-intensive, and it should not compete with writes and queries. A compactor is not interchangeable with a reader or a writer: it compacts *and*, in Pattern 1, moves the resulting data between nodes. A cluster deployed without one is a misconfiguration, not a supported shape.

If that is the rule, the writer fallback is arguably the wrong behaviour: it papers over a missing compactor by quietly loading compaction onto a node that is already taking ingest, and it makes "which node is compacting" depend on cluster history rather than on roles.

## Why it is not urgent

Losing compaction is a **degradation, not an instability**. Writes still land, queries still answer; they get slower because the Parquet layout stops being consolidated. Arc already reports the condition, rate-limited, and names both remedies:

> No compactor elected: compacted files will accumulate. Set `ARC_CLUSTER_ROLE=compactor` on one node and restart, or enable automatic failover (`cluster.failover_enabled`).

That is the line this project draws: warn about degradation, refuse only what is unrecoverable. (See #894 and #892, both closed as not-a-bug on exactly this reasoning.)

## What changes if the answer is "compactor-only"

The change itself is small, and #876 was built so it would stay small. `canHoldCompactorLease` is pinned to **exactly** the set `selectNewCompactor` can choose from, with `TestEveryAssignableRoleIsAlsoSelectable` asserting the two cannot drift. That invariant exists because a lease must only ever land somewhere an automatic path could also place it — otherwise losing that node leaves the lease on a corpse nothing can move, and `compactionClusterGate` switches from the static role check to the lease the moment it is non-empty, so compaction would stop cluster-wide with no recovery. Narrowing the predicate to `RoleCompactor` and narrowing the selector together is a two-line change plus that test.

The consequences are the part worth thinking about:

- **A single compactor becomes a single point of failure for compaction.** Two compactor nodes become the HA answer, the way three writers are for ingest. Worth deciding whether that is the recommendation, and whether `checkCompactorElected`'s "multiple compactors elected" warning should then become "fewer than two".
- **#876's preemption becomes the migration mechanism and then goes inert.** A cluster that already has a writer holding the lease — every cluster upgraded past #870 — needs preemption to move it onto the compactor. After that, a writer could never hold it, so preemption would have nothing to preempt.
- **The manual endpoint narrows.** `POST /api/v1/cluster/compactor/assign` would stop accepting writer targets, which removes the "move compaction off this node right now" lever for a cluster with only one compactor.
- **Pattern 1 vs Pattern 2 may differ.** In Pattern 2 the compacted output lands in shared storage and any node can serve it. In Pattern 1 the compactor also transits files between nodes, and `OriginNodeID` routes replica pulls back to whoever produced the output — so a writer holding the lease there is serving compaction I/O on top of ingest *and* peer fetches. If the two patterns want different answers, say so explicitly rather than having one fall out of the other.

## Open questions

1. Compactor-only everywhere, or compactor-only in Pattern 1 and writer-fallback in Pattern 2?
2. If compactor-only: does Arc recommend two compactor nodes, and should the health warning change to reflect that?
3. Does the manual assign endpoint keep a writer escape hatch for deliberate operator overrides, and if so how does it avoid placing the lease where nothing automatic can retrieve it?
4. Is there a case for the lease being *unassigned* rather than held by the wrong node — i.e. making "no lease" mean "lease-managed but nobody eligible", distinct from today's "no lease means fall back to static role"? That distinction came up in #876's review and was deferred; it is the same question in different clothing.

Related: #390 (introduced the writer fallback), #876 (preemption, the manual endpoint, and the eligibility invariant), #884 (the gate is re-checked every tick), #870 (the chart's compactor pod never joined), #894 / #892 (both closed — the existing warning is the answer for a missing compactor).

Contributor guide

Open the contributing guide

Research direction

Start with CompactorFailoverManager.selectNewCompactor, canHoldCompactorLease, TestEveryAssignableRoleIsAlsoSelectable, and compactionClusterGate; then read related issues #876, #390, #884, #894, and #892. Compare the Pattern 1 and Pattern 2 consequences, the manual assign endpoint, and the health warning. Done means the product decision and its supported role, recovery, and test implications are explicitly recorded.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.