d-c-manning commented on PR #7552:
URL: https://github.com/apache/hbase/pull/7552#issuecomment-6430627590
Thanks for sticking with this, Swarali. The updated root-cause section is
much clearer than the earlier explanations, and I agree with most of it. I do
think the patch as it stands narrows the NPE but doesn't fix the reported
problem, which is compaction staying dead until the region is reopened.
### What I agree with
- A compacted file can sit in `compactedfiles` with a null reader.
`CompactedHFilesDischarger` → `removeCompactedfiles` →
`HFileArchiver.moveAndClose` calls `closeStoreFile` before the rename. The file
only leaves `compactedfiles` in `clearCompactedfiles`, after the whole batch is
archived. All of this runs under `archiveLock`, not the store write lock.
- Only date-tiered hits it, because `SEQ_ID_MAX_TIMESTAMP` is the only
comparator that reads the timestamp from the reader.
- One more detail for the description, since the test relies on it:
`GetMaxTimestamp` is only reached when `getMaxSequenceId()` ties. That is
routine for date-tiered, because `AbstractMultiFileWriter.commitWriters` stamps
every output window of one compaction with the same maxSeqId.
### Why the wedge isn't fixed
In `HStore.removeUnneededFiles`:
1. `addToCompactingFiles(delSfs)` puts the expired files in
`filesCompacting`.
2. `replaceStoreFiles(...)` is called.
3. The only thing that removes them from `filesCompacting` is the
`actionUnderLock` lambda, which runs *after* `addCompactionResults` in
`StoreEngine.replaceStoreFiles`.
There is no `finally`. Compare `HStore.compact()`, which has `finally {
finishCompactionRequest(cr); }`.
`addCompactionResults` also isn't atomic. By the time the `compactedfiles`
sort throws, `storeFiles` has already been reassigned and the files are already
`markCompactedAway`. So on any exception there:
- The expired files are in neither `storeFiles` nor `compactedfiles`, so
they are never archived.
- They are permanently stuck in `filesCompacting`.
- Every later `getCurrentEligibleFiles` hits
`Preconditions.checkArgument(idx != -1)`, which is the
`IllegalArgumentException` in the JIRA.
Reopening is the only thing that clears it.
The comparator guard removes one way to get there. Any other exception
between `addToCompactingFiles` and `actionUnderLock` produces the same
permanent wedge and the same orphaned files. For example, an `IOException` from
`getLiveFiles`, or a comparator contract violation (see below). HBASE-29875 is
another report of the same "compaction dead until reopen" symptom, from a null
reader on a different path (`getUnneededFiles`). So the failure mode isn't
specific to date-tiered.
### Problems in the comparator change
1. **It's still racy.** `sf.getReader() == null` and then
`sf.getMaximumTimestamp()` are two separate reads of a volatile field that the
discharger nulls without the write lock. The window is tiny, but it's the same
NPE. Reading once avoids it:
```java
StoreFileReader r = sf.getReader();
return r == null ? Long.MAX_VALUE : r.getMaxTimestamp();
```
`StoreFileReader.getMaxTimestamp()` already defaults to `Long.MAX_VALUE`
when `timeRange` is null.
2. **The sort key can change mid-sort.** A file's key jumps to
`Long.MAX_VALUE` when its reader closes concurrently. If that happens during a
sort, the comparator violates its contract. TimSort can throw "Comparison
method violates its general contract" on larger lists, and that is another
exception on the same unprotected path. It's unlikely, but a key that doesn't
depend on reader lifecycle is the principled fix. One option is to cache the
max timestamp in `HStoreFile` at open time, like `sequenceid`, `firstKey` and
`lastKey`. The other is to stop sorting `compactedfiles` at all. I couldn't
find a consumer that depends on its order, and `removeCompactedFiles` re-sorts
it anyway.
3. **The try/catch/log/rethrow in `addCompactionResults` doesn't change
behavior.** It just logs the error twice.
### Description and test don't match the diff
- The description says `HFileArchiver.moveAndClose` was reordered to rename
first and close only on success, with tests for it. Neither is in the diff,
which touches only `DefaultStoreFileManager`, `StoreFileComparators` and
`TestHStoreFile`. Either add that change, which I think is worth doing (it
removes the root cause of the null-reader window), or update the description.
- The new test simulates a stranded file after a failed archive. It doesn't
exercise the race, and it doesn't check recovery. It asserts that the sort
doesn't throw, but not that compaction can still be selected afterwards.
### Suggested direction
1. In `HStore.removeUnneededFiles`, wrap `replaceStoreFiles` so that on
failure `delSfs` is removed from `filesCompacting`, mirroring
`finishCompactionRequest`. This is the fix for the "never recovers" symptom.
2. Make `DefaultStoreFileManager.addCompactionResults` build both new lists
before assigning either, and only then assign them and call
`markCompactedAway`. A failure then leaves state untouched instead of orphaning
files.
3. Keep a null-safe comparator, but with a single read, or a cached
timestamp.
4. Add a regression test that forces the TTL-path replace to fail, then
asserts `filesCompacting` is clean and `requestCompaction()` still selects
files.
5. Optionally, do the `moveAndClose` reorder the description mentions.
Happy to help test any of this.
--
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]
