quickwit icon indicating copy to clipboard operation
quickwit copied to clipboard

Fix the split store

Open fulmicoton opened this issue 1 year ago • 5 comments

Is the split store broken?

fulmicoton avatar Sep 27 '22 13:09 fulmicoton

Analysis

After landing https://github.com/quickwit-oss/quickwit/pull/1980 we will have only one merge pipeline for all pipelines. It behaves as follows.

If a split is mature, we do not cache it. If a split is mature AND we have capacity we cache the file. If we don't we do not cache the file (simple). Upon a successful merge, the idea was to remove the split upon a successful merge. however we never call split_delete is never called. That deleting code used to exist and was in the regular GC. We would suppress files from the local cache before deleting them from the storage. This is THE bug. I think it was done with my benediction under the false memory that the split store is a LRU cache.

We however have some gc-like logic that takes the list of splits from the metastore and removes all of the splits in the local store that are NOT within the metastore. (we call those dangling) These gc is only triggered when the indexing pipeline is spawned.

LRU might not be the panacea, but we can go for it if necessary. Detail: Our access pattern is actually an anti use-case for LRU eviction. We perform merge over the oldest split first, and then we never access the item again if a merge is successful If the merge is not successful we are likely to access it very rapidly.

If the cache is a tiny bit to small to host all of the splits we are interested in, then the situation directly from 100% hit rate to 0% hit rate (for the older generation), which needless to say is not ideal. Our cache actually would benefit from an MRU policy... Unfortunately and MRU policy does not guard us from dangling split.

Solution

Quickfix: We could switch to an LRU cache despite the MRU-friendly access pattern. Fortunately the generational aspect of merges, and the fact that we merge several splits together should mitigate the problem.

Possibly smarter fix:

  • LRU + actively delete stores from the split when they are removed from the storage.
  • Should the merge policy call retain_splits to the split store from time to time?

Fake LRU... BinaryHeap over the split_ids. No update on access.

Other problems

We don't clean the split store of an index that has been removed.

fulmicoton avatar Oct 06 '22 08:10 fulmicoton

@evanxg852000 I think @guilload had some clear idea on the solution.

fulmicoton avatar Oct 06 '22 15:10 fulmicoton

Actually I'll pick that ticket @evanxg852000 and tackle it tomorrow

fulmicoton avatar Oct 06 '22 15:10 fulmicoton

Some pointers for a solution:

  • one global split store per node held by the indexing service.
  • split store is responsible for enforcing the space quota.
  • on startup, the split store GCs the dangling indexes AND splits.
  • split store keeps track of splits in a binary heap of split IDs; since split IDs are ULIDs, the split store is aware of the order of creation of the splits
  • when the space quota is reached, the split store evicts at the top of the heap. (I wonder whether we should also take the size of the splits into account since the cost of downloading is higher for those cache misses)

Some scenarios seem to indicate that we need to run the GC logic periodically:

  • index or split deletions
  • indexing pipeline(s) for an index no longer scheduled on node
  • indexing pipeline(s) complete successfully (backfill)
  • crashed indexing pipelines stuck in a retry loop
  • ...

There are also cases where a split can stay in limbo; for instance, a split will never be merged (index still exists, but indexing pipelines are now scheduled on another node, and no incoming documents). We may want to evict splits older than some threshold. FTR, for almost all the components of the data platform stack at Airbnb (Airflow, YARN, Spark shuffle service, ...), we ended up putting a CRON job in place to remove temporary files older than x hours.

Refinements

  • avoid caching mature splits
  • evict merged splits

guilload avatar Oct 06 '22 21:10 guilload

Finding other problems with the current code. Right now we do move splits out of the store when they are accessed, but we don't update stats at this point. This is not a fundamental problem.

fulmicoton avatar Oct 07 '22 09:10 fulmicoton