Quarantined bookies are never released early when they become healthy again
- Dominant language
- Java
- Stars
- 2k
- Forks
- 976
- Avg merge
- 6d 15h
- Merged PRs (30d)
- 7
Description
**BUG REPORT**
***Describe the bug***
A quarantined bookie is only released when a fixed timer expires. Nothing releases it when the bookie becomes healthy again.
`BookieWatcherImpl` builds the quarantine set as a write-expiring cache:
```java
this.quarantinedBookies = CacheBuilder.newBuilder()
.expireAfterWrite(conf.getBookieQuarantineTimeSeconds(), TimeUnit.SECONDS)
.removalListener(...)
.build();
```
The only other way out is `releaseAllQuarantinedBookies()`, which `BookKeeper.checkForFaultyBookies()` calls solely when the metadata-driver health-check flag has been switched off — i.e. when the whole feature is disabled, not when a particular bookie recovers.
Entry into quarantine is comparatively cheap:
```java
if (pool.errorCounter.getAndSet(0) >= bookieErrorThresholdPerInterval) {
faultyBookies.add(pool.address);
}
```
(`BookieClientImpl.getFaultyBookies()`), evaluated every `bookieHealthCheckIntervalSeconds`.
So a bookie that exceeds the error threshold inside a single check interval — one long GC pause, one brief network blip — is excluded from ensemble placement for the whole `bookieQuarantineTimeSeconds`, with no way to re-admit it early even though the very next interval shows it clean. Deployments that raise the quarantine time to reduce flapping make the asymmetry worse: the penalty for one bad interval grows while the recovery signal is still ignored.
Quarantine is soft — `newEnsemble()` and `replaceBookie()` both retry with an empty exclusion set on `BKNotEnoughBookiesException` — so this does not hard-fail placement. But it does silently degrade it: the fallback ("Not enough healthy bookies available, using quarantined bookies") is logged only at DEBUG, so an operator sees the initial quarantine WARN and then nothing until expiry.
***To Reproduce***
1. Enable the bookie health check on a client.
2. Cause one bookie to return more than `bookieErrorThresholdPerInterval` errors within a single `bookieHealthCheckIntervalSeconds` window, then let it return to normal immediately.
3. Observe `Bookie has been quarantined because of read/write errors.`
4. Observe that the bookie is excluded from new ensembles for the full `bookieQuarantineTimeSeconds`, and that `Bookie is no longer quarantined` only appears when the timer expires — regardless of the bookie being healthy for the entire period.
***Expected behavior***
Release a bookie from quarantine once it demonstrates health — for example, invalidate its entry when a subsequent health-check interval finds its error count below the threshold, or require N consecutive clean intervals before re-admitting. The fixed timer would then be an upper bound rather than the only mechanism.
***Additional context***
Separate but adjacent, and easy to misconfigure: `bookieQuarantineRatio` reads like a cap on how much of the fleet may be quarantined, but it is a per-bookie, per-check probability:
```java
for (BookieId faultyBookie : faultyBookies) {
if (Math.random() <= bookieQuarantineRatio) {
bookieWatcher.quarantineBookie(faultyBookie);
...
```
(`BookKeeper.checkForFaultyBookies()`). It staggers the decision across clients — which matches its documented intent — but it places no bound on how many bookies a single client can quarantine, and with a short check interval a persistently faulty bookie is quarantined within a couple of intervals anyway. A documentation clarification, or an actual cap, would prevent the setting from being relied on for something it does not do.
Contributor guide
Assessment
This issue has not been assessed yet.