Add thread to periodically perform pending cache maintenance

[owenhalpert]

Description

Adds a ScheduledExecutor class that, for each cache, takes in a Runnable with a call to cleanUp and periodically executes according to the values of KNN_CACHE_ITEM_EXPIRY_TIME_MINUTES and QUANTIZATION_STATE_CACHE_EXPIRY_TIME_MINUTES. This will perform any pending maintenance (such as evicting expired entries) which was previously only performed when the cache was accessed. The maintenance thread is created whenever a NativeMemoryCache or QuantizationStateCache is instantiated or rebuilt and can be shut down with either class's close method. Relevant logic for cleanup was added to some testing base classes.

Related Issues

Resolves #2239

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[kotwanikunal]

@owenhalpert The changes look good in general. What I'd be interested in is testing under load for a resource constrained system - can you verify if this adds to the latency or impact the performance in any way?

We did implement a force evict before writes with #2015. Can you also enable this feature flag and run the above tests to ensure it behaves well?

[kotwanikunal]

You can also avoid maintaining the Cache object reference here by using a functional interface. That would also get rid of the generification of this class.

Simply pass and store the runnable reference instead of the cache as new CacheMaintainer(() -> cache.cleanUp());

Possibly also move this class to the util package and call it a ScheduledExecutor with Runnable ref and interval as parameters.

public class ScheduledExecutor implements Closeable {

...

  public ScheduledExecutor(Runnable reference, long scheduleMillis) {
  ...
  }

  ...
}

[owenhalpert]

@kotwanikunal what do you think about creating the executor and calling scheduleAtFixedRate within each cache class instead of creating a new ScheduledExecutor class?

[owenhalpert]

@kotwanikunal I've completed the benchmarking on a single node cluster limited to 3GB of memory.

Before benchmarking my code, I validated the CacheMaintainer was actually running by inspecting the OpenSearch process status in the Docker container and found that after about a minute, the RssAnon value would decrease by about 0.5gb without any interference on my part, signaling the maintainer successfully cleaned up the expired entries. This is aligned with what I saw on the standard maintenance on the clean code (which I triggered manually by accessing the cache).

Results below:

Performance Summary for Resource-Constrained Testing (3GB Memory Limit)

Run 1: Clean 2.18 code

• Indexing:
  • p50 latency: 16.96 ms
  • p90 latency: 32.47 ms
• Search:
  • p50 latency: 330.48 ms
  • p90 latency: 437.78 ms

Run 2: Clean 2.18 code, Force evict ON

• Indexing:
  • p50 latency: 17.27 ms
  • p90 latency 33.82 ms
• Search:
  • p50 latency 337.43 ms
  • p90 latency 428.70 ms

Run 3: PR changes added, Force evict ON:

• Indexing:
  • p50 latency: 16.89 ms
  • p90 latency: 32.24 ms
• Search:
  • p50 latency: 341.38 ms
  • p90 latency: 414.06 ms

Run 4: PR changes added, Force evict OFF:

• Indexing:
  • p50 latency: 17.43 ms
  • p90 latency: 34.63 ms
• Search:
  • p50 latency: 346.83 ms
  • p90 latency: 430.04 ms

This suggests there is no significant impact on latency with my code changes. I've included the full results of these test runs here:

https://gist.github.com/owenhalpert/05ad4f5ae9577f717f2c59f2039d52e4

[jmazanec15]

Minor comments overall looks good

[jmazanec15]

Why swallow exception here?

[owenhalpert]

Any exceptions from Guava cache operations would otherwise halt our scheduled executor. If an exception occurs here, it would be from Guava's internals rather than our logic, so per Dooyong's suggestion above we can log it and continue scheduling cleanup tasks.

This way we can ensure the cache maintenance keeps running even if individual cleanup attempts fail, and we can monitor Guava errors in the logs.

[shatejas]

This seems to be creating a new thread for each instance, I wanted to understand the thought process behind this.

I understand that this is used for cache so the threads won't grow exponentially, but since its in util package and the name is generic - any misuse of this can create unbounded number of threads with each new instance. Can we be a bit more defensive here?

The suggestion here is to refactor and make sure there are fixed number of threads for cache cleanup across caches