[GCP] Garbage collection

[AlCutter]

This PR adds a "garbage collector" to the GCP storage implementation which periodically removes partial tiles & entry bundles which have become obsoleted by corresponding full versions as the log has grown.

See #672 for corresponding changes to the client code.

Towards #670

[mhutchinson]

We agreed that this would be a post-beta feature. I think the order of operations should be:

• client code change should go in
• tag beta
• add non-blocking features like this

My concern is that a) this is new code that risks performance degradation, and b) we'll end up delaying beta release until we've written and tested an equivalent feature for the other storage implementations.

Thoughts?

[AlCutter]

We agreed that this would be a post-beta feature. I think the order of operations should be:

• client code change should go in
• tag beta
• add non-blocking features like this

My understanding that beta is going to be tagged ~this week.
I'm ok to delay merging this until after tagging beta if that is the case, but I'd appreciate a review in case there are changes to make in the meantime.

The client code is already in #672, which should of course go in first so that clients are ready to cope with logs which GC before this change is merged.

My concern is that a) this is new code that risks performance degradation, and b) we'll end up delaying beta release until we've written and tested an equivalent feature for the other storage implementations.

Since this isn't a beta-blocking feature, I'm not sure I understand why you'd want to block beta for the other storage implementations?

Personally, I'm not overly concerned about potential for performance regression (all PRs have that potential), but I can see an argument for delaying merging this until post-beta for correctness purposes; I believe it's correct, and it's fairly well covered with fsck tests, but it is after all making structural changes to the log it manages.

[mhutchinson]

My understanding that beta is going to be tagged ~this week. I'm ok to delay merging this until after tagging beta if that is the case, but I'd appreciate a review in case there are changes to make in the meantime.

Sure, sounds good. I'll take a look at it, but we're aligned that we won't merge until beta is tagged.

[mhutchinson]

I've been deciphering this loop for over 10 minutes now. I think some docs would be beneficial:

• describe the strategy: this outer loop starts at the bottom level (l = 0) and works its way up the levels until either we delete the max number of tiles (handled inside the loop, by breaking), or we climb past the top of the tree (x > 0). Within each level, we traverse left to right, clearing out garbage.
• briefly describe l, f, x and their invariants
• that this loop handles both the entries paths and tile paths at the same time, but really treats entries as a special case at level 0 of the tile paths

[mhutchinson]

I can't convince myself that this loop doesn't leave garbage in the upper layers, but still move along the from size in the coordinator. If true, this means that garbage could be permanently left up there. I think it stems from this relationship between this increment of fromSize and the breaking based on d > maxDeletes above.

Imagine maxDeletes is set to 1. We will always stay at the bottom level of the tree, deleting one thing from entries, and one thing from tile paths (which is 2, technically breaking the "max" deletes, but I think that's fine as long as we're honest in the docs that "max" is a guideline, not a rule). Then we will break the loop, above, and then continue to increment the coordinator. After a sufficient number of repeats of this, we will have bumped fromSize to treeSize and no work will be performed. But we never went to the upper floors.

Have I missed something?

I think it will also skip upper layer as soon as it finds a tile where ri.Partial != 0, which is almost guaranteed after traversing all the way to the right.

My intuition is that the iteration should prioritize traversing up the tree before fully GCing the lower levels. i.e. keep fromSize from being updated until all partial tiles that could have been generated in all levels of the tree have been found and collected.

[AlCutter]

I've switched it a "vertical" implementation - I'd originally discounted that approach in order to avoid repeatedly considering higher level tiles, but avoiding that is far easier than I assumed it would be.

[mhutchinson]

I discuss this in the comments below, but maxDeletes required me to update my intuition. In addition to the non-strict nature of it, my expectations about what it counted as a deletion were wrong.

What I expected was that this would control the maximum number of partial tiles that would be deleted. It actually controls the number of calls to deleteWithPrefix, each of which could delete up to 255 partial tiles. Nothing wrong with this, but worth having a doc line to make it clear.

[AlCutter]

Changed to be "how many bundles should be considered for GC before exiting"

[mhutchinson]

It's an internal interface, so I'm not overly worried, but this looks like a strange cousin next to these other 2 get/set methods.

[AlCutter]

Open to alternative suggestions, but the thinking was that this enables infra to do this specific task in the best way it can (e.g. POSIX could potentially do an rmdir()) rather than force all to conform to an LCD like findObjectsWithPrefix and deleteObject