CASSANDRA-21649: Ensure SSTable files are deleted in order of last-updated timestamp

[nivykani]

When a node restarts after crashing, we need to clean up any partially-deleted SSTables, but we do a safety check on REMOVE log records to verify that the last-modified timestamps match the actual files on disk. If the timestamps don’t match, we throw an error that the log is corrupted, and the node doesn’t start.

The comment in LogFile.verifyRecord() states: "Because we delete files from oldest to newest, the latest update time should always match.” But this isn’t always true; regular SSTable deletion after compaction always deletes DATA files first, and doesn’t explicitly set the deletion order of other files.

This patch ensures SSTableTidier deletes files in last-modified order, so the overall timestamp never changes on partial deletes. We look up all the file timestamps once, sort the list, and then delete in order.

The Cassandra Jira

[maedhroz]

deep-review is happy so far...reviewing manually...

[maedhroz]

Would it actually be possible to move the delete() method the patch modifies to AbstractSSTableFormat?

Yes, this specific method is a good candidate — the private helper the patch touches:

  private void delete(Descriptor desc, List<Component> components)
  {
      logger.info("Deleting sstable: {}", desc);
      Map<Component, Long> lastModified = new HashMap<>();
      for (Component c : components)
          lastModified.put(c, desc.fileFor(c).lastModified());
      components.sort(Comparator.comparingLong(lastModified::get));
      for (Component component : components)
      {
          logger.trace("Deleting component {} of {}", component, desc);
          desc.fileFor(component).deleteIfExists();
      }
  }

references zero instance state from BigFormat/BtiFormat — only its parameters and Component/Descriptor, both public in the parent org.apache.cassandra.io.sstable package, already visible to AbstractSSTableFormat. It's byte-for-byte identical in both classes right now. Nothing blocks moving it verbatim
into AbstractSSTableFormat as a protected final method.

Two things to decide, not blockers:

1. Logging. Each class currently has its own private static final Logger logger = LoggerFactory.getLogger(BigFormat.class) / (BtiFormat.class). Hoisted as-is, log lines would attribute to AbstractSSTableFormat instead of the concrete format. Fix is trivial — swap to LoggerFactory.getLogger(getClass()) in
   the base method, which resolves per-instance at negligible cost — but it's a deliberate choice, not automatic.
2. Where it should live. The sibling method deleteOrphanedComponents is already deduplicated in exactly this situation (identical one-liner in both formats: SortedTableScrubber.deleteOrphanedComponents(descriptor, components);) — but the codebase's chosen pattern for that was a shared static utility
   method (on SortedTableScrubber), not a concrete method on AbstractSSTableFormat. That's precedent worth following for consistency: add this as a static method (e.g. SortedTableScrubber.deleteComponentsOldestFirst(desc, components) or similar) rather than putting file-deletion I/O logic directly on the
   abstract base, which otherwise only holds name/options/equals/hashCode — pretty thin and administrative. Either approach works; matching the existing deleteOrphanedComponents precedent is the more consistent choice.

The public delete(Descriptor) wrapper (not touched by this patch) can't be fully unified the same way — BigFormat's does key-cache invalidation that BtiFormat's doesn't (BTI has no key cache) — but that's orthogonal to the private helper you're asking about.

[nivykani]

I agree, it makes sense to dedupe. On 2: although there's a precedent that deleteOrphanedComponents already lives in SortedTableScrubber, it doesn't seem related to scrubbing. It seems more intuitive to move both methods to AbstractSSTableFormat, even though currently it's a minimal class. Updated this patch to make this change.

For the logging, I had to use getClass().getSimpleName() in the log message instead of LoggerFactory.getLogger(getClass()) to get it to show the subclass, but still a minor change.

[maedhroz]

Left one comment about something I think we can move up to the format base class, but otherwise LGTM