MINOR: clarify why suppress can sometimes drop tombstones

[vvcephei]

Clarify when and why suppress would drop tombstone records. No behavioral change.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[vvcephei]

A contributor asked me what shouldSuppressTombstones is for, and I couldn't remember. After figuring it out, it was clear that this logic needs clarification.

Please take a look when you get the chance, @guozhangwang @mjsax @bbejeck @ableegoldman

[mjsax]

Retest this please.

[mjsax]

Retest this please.

[jonathanpdx]

Consider renaming to safeToDropTombstones:

Suggested change

	boolean shouldSuppressTombstones() {
	boolean safeToDropTombstones() {

[vvcephei]

Ah, missed one. Thanks!

[vvcephei]

retest this, please

[guozhangwang]

LGTM. Left a meta comment which is actually out of the scope of this PR.

[guozhangwang]

Sounds good.

Actually I've seen the same situation for our current caching layer flushing logic as well: e.g. put(A, v) -> delete(A) and both only hit the cache layer. When flushing we tried to read the old value and found its null bytes, so we know nothing was flushed for A and nothing written to downstream before so we can skip putting a tombstone to underlying store as well as downstream.

For suppression buffer though, it is harder since you do not have an underlying store to fetch the old value, and of course reading the whole changelog to see if there's any updates on this key A costs you everything. But suppose we always have a persistent buffer, this may be an easier task.

[vvcephei]

Thanks @guozhangwang ,

Yes, I think that's why the record cache keeps a set of "dirty keys", just like the suppression buffer. If you have a dirty-key, but don't find any value for it, then you know it was a delete and you can send a tombstone.

It is indeed harder for the suppression buffer, since we try not to completely duplicate all the data. I guess it's not all the data, but it is all the keys at least. For example, let's say we store the data as the heterogeneous type [nonTombstoneValueHasBeenEmittedFlag] | [nonTombstoneValueHasBeenEmittedFlag, valueToBeEmitted]. Then, when we get a tombstone, if the pre-existing value has nonTombstoneValueHasBeenEmittedFlag == true, we know we must emit the tombstone. If there is no value, or if nonTombstoneValueHasBeenEmittedFlag == false, we know that we don't need to send the tombstone. Upon sending the tombstone, we would simply delete the record from the store. Upon emitting a non-tombstone value, we would drop the value from the store and only store [nonTombstoneValueHasBeenEmittedFlag := true].

Not sure I would do this in memory, but as you point out, it could be an option for the persistent version. I guess if you think the whole "live" key space would fit in memory (plus one bit per key for the flag), then in-memory would work too.

[guozhangwang]

Right. But note that the current dirty-key in cache is not enough determining if we have, ever, write for a key to the underlying store which is not deleted yet: dirty-key only contains the dirty-keys since last flush, i.e. the key not in the dirty-key is only a necessary, not sufficient condition. And that's why we can only consider not writing the tombstone if the read on this key returns null-bytes, indicating nothing was there.

[vvcephei]

Oh, right, I guess we would need to do something like what I described above, or some equivalent solution...

[bbejeck]

Thanks for the clarification, LGTM

[vvcephei]

Some core integration test failed...

Retest this, please.

[guozhangwang]

I've ran the unit tests locally and it passed.

[vvcephei]

Thanks, @guozhangwang !