HBASE-29890 WAL tailing reader should resume partial cell reads instead of resetting compression

[sidkhillon]

When the WAL tailing reader hits EOF mid-cell during WAL compression, it currently returns EOF_AND_RESET_COMPRESSION, which forces the reader to re-read the entire WAL file from the beginning to rebuild dictionary state. This is an O(n) operation that becomes increasingly expensive as the WAL grows.

The root cause is that the CompressedKvDecoder eagerly adds entries to the compression dictionaries (ROW, FAMILY, QUALIFIER, and tag dictionaries) as it reads each field of a cell. If an IOException occurs partway through reading a cell, the dictionaries are left in a partially-updated state that no longer matches the actual stream position. The reader has no choice but to throw away the entire compression context and start over.

Proposed Fix is to defer dictionary additions until a cell is fully parsed:

• Buffer ROW/FAMILY/QUALIFIER dictionary additions in CompressedKvDecoder and only commit them after parseCellInner() succeeds. On IOException, discard the pending additions.
• Add a similar deferred-addition mode to TagCompressionContext for tag dictionaries.
• Reset the ValueCompressor if an IOException occurs during the value decompression phase.

With deferred additions, hitting EOF mid-cell leaves the dictionaries in the state they were after the last fully-read cell. This means the reader can return EOF_AND_RESET (a cheap seek to the saved position) instead of EOF_AND_RESET_COMPRESSION, and resume reading from where it left off once the file grows.

[sidkhillon]

cc @Apache9, I would really appreciate any feedback you have on this PR. Thank you!

[Apache9]

Is this algorithm always right?

The Dictionary interface does not guarantee that the newly added entry will have the current size as its index, and in the implementation, LRUDictionary may move entries when it is being accessed...

[sidkhillon]

That's a good point. I have created an UndoableLRUDictionary that will let us rollback changes to the dictionary, and I've added many scenarios to test it out as unit tests. Please let me know how that looks.

[sidkhillon]

@Apache9 I've addressed the PR feedback you left, and I would really appreciate another review when you have the chance. Thank you!

[hgromer]

I wonder if doing this in 2 phases avoids needing an n2 approach to iteration here.

Additionally, rollback() does remove/setContents/put on nodeToIndex (a content-based HashMap) one node at a time, in non-deterministic IdentityHashMap iteration order. If during the tracked period a node gets overwritten with a value that equals another node's original value (e.g., evict "a" from slot 0, then "a" gets added to slot 1), there's an intermediate state during rollback where two nodes hold the same content. Depending on iteration order, the HashMap operations can clobber each other, leaving a missing entry in nodeToIndex.

Would it be more efficient and safer to do:

// Phase 1: restore all node contents
  for (...) {
      node.prev = snap.savedPrev;
      node.next = snap.savedNext;
      if (snap.savedContents != null) {
          node.setContents(snap.savedContents, snap.savedOffset, snap.savedLength);
      }
  }
  // Phase 2: rebuild nodeToIndex from scratch
  backingStore.nodeToIndex.clear();
  for (short i = 0; i < savedCurrSize; i++) {
      backingStore.nodeToIndex.put(backingStore.indexToNode[i], i);
  }

[sidkhillon]

Thanks, I've implemented this suggestion.

[hgromer]

It seems we're no longer using dictType here. Is this okay? Callers might expect the underlying dictionary type to be dictType. So if it has any custom behavior, you'd be overriding/removing it. Is this okay? What's the intended case for this pluggable dictionary type?

[sidkhillon]

dictType is not really used beyond setting it to the default LRUDictionary in the codebase, so to avoid confusion, I removed the argument from the constructor and set the type explicitly.