IndexingScheduler: fetch and import in batches #24285

VyrCossont · 2023-03-27T17:22:13Z

This fixes an issue that @supernovae and I encountered on Universeodon: when a Redis set for Mastodon's Chewy import strategy (like chewy:queue:StatusesIndex) becomes extremely large (over 2M IDs in our case¹), trying to bulk-import so many objects at once causes the Sidekiq job to run out of memory and die after importing few to no objects and removing none of their IDs from the set, causing the job to make no progress and the set to never decrease in size.

This patch does two things to make IndexingScheduler more robust:

Replaces the single SMEMBERS command with several SSCAN commands (see SCAN for more detail), limiting to 10,000 the number of IDs returned from Redis at once. This results in several more Redis calls to retrieve the whole set, but smaller responses. The COUNT argument is treated as a hint, but the returned number of responses should be the same order of magnitude for large collections. Deleting already-seen elements from a set during a scan should not cause issues with the scan; see the "Scan guarantees" section of the SCAN docs.
More critically, this limits the number of objects imported into Chewy at once, reducing peak memory usage significantly. The IMPORT_BATCH_SIZE number of 100 worked for us, but is conservative, and it's possible that increasing it up to SCAN_BATCH_SIZE or increasing both would result in higher throughput; it'd be useful to know the size of chewy:queue:StatusesIndex on other large production instances to tune it. The important thing is that the import batch size is capped instead of being potentially unbounded.

TODOs to exit draft:

Increase batch sizes to known-safe values from large production instances

This was mostly due to a bug in Universeodon's extended search functionality. That bug has already been fixed in Extended status and account search #24055, but the fix was inadvertently removed from Universeodon during a merge, causing status indexing to fail for some statuses. That said, sufficiently active instances with future extended search functionality may well end up with large chewy:queue:StatusesIndex sets due to normal use, or if the Sidekiq process running IndexingScheduler is down for an extended period of time. ↩

VyrCossont · 2023-03-27T17:29:23Z

For reference, a typical chewy:queue:StatusesIndex size on Universeodon during normal operation is a few hundred up to about 2000.

VyrCossont · 2023-03-28T16:28:25Z

vmst.io contributed their numbers, which peak around 600 queued statuses.

ClearlyClaire · 2023-03-29T11:34:29Z

app/workers/scheduler/indexing_scheduler.rb

+          redis.pipelined do |pipeline|
+            ids.each { |id| pipeline.srem("chewy:queue:#{type.name}", id) }
+          end


I wonder if it wouldn't be better to pass all ids at once.

I also wonder if it wouldn't be better to rewrite the whole thing to use spop instead. On the one hand, a reason not to use it would be error handling, but on the other hand if errors pile up it does not make sense to let the Redis set grow uncontrollably either.

I wondered why the original didn't use the batch form of SREM. Maybe there's no reason?

The SPOP approach could work, and prevents the queue from growing without bound, especially in scenarios where an error is unrecoverable.

The one wrinkle there is that the existing code handles short transient ES outages well, and I wouldn't want to change that property casually if instances are accidentally relying on it.

FWIW, Universeodon didn't suffer any obvious ill effects from having 2M status IDs queued for indexing, and once the bug was fixed and this patch was applied, it cleared the backlog successfully.

Just pushed a change to make the import batch size larger and use batched SREM, since neither would change the behavior on error.

nschonni · 2023-03-29T18:36:08Z

app/workers/scheduler/indexing_scheduler.rb

@@ -6,17 +6,17 @@ class Scheduler::IndexingScheduler

  sidekiq_options retry: 0

+  IMPORT_BATCH_SIZE = 1000


Wonder if this would make sense as a namespaced environment variable

Maybe? Mastodon doesn't break out the majority of its limits and tuning constants, but I don't know if that's house style or what.

I think this kind of tuning is marginal enough to not warrant adding an environment variable. That can be revisited in the future if the need arises.

app/workers/scheduler/indexing_scheduler.rb