Interested in refactoring or rewriting the cache?

While checking the source code, I wrote some notes and thought I might as well share them with you.

Are you interested in refactoring the caching code (as most of this issue highlights), or rewriting it (as the first section suggests), or is it not a high-priority concern for now?

(I think this project is interesting and I am willing to help. Eh, in case you are wondering, I'm a software engineering graduate who "mains" JavaScript and Java.)

The Interface

• CacheProvider interface is fine, but what about using some standard like the Java Caching API (JCache)?
  There are a few popular and open-source implementations like Ehcache and Hazelcast.
  If you don't want to use a specific implementation, we could just remove our CacheProvider and use jCache's interface with our existing provider implementations.
  The user will be able to use our built-in cache providers or their more advanced provider and integrations (including Spring, JEE, and whatnot).

TLDR: Caching is super hard. We should use standards or standard/mainstream libraries if they serve our needs.

TieredCacheProvider

It is a wrapper around other providers.
It forwards changes to all providers and returns data from the first provider.
It does not propagate setTTL commands.

EmptyCacheProvider

The name is kinda confusing and does not really suggest what it does. Maybe rename it to:

• NoopCacheProvider because all of its methods are no-op.
• NullCacheProvider because it behaves according to the Null Object pattern.

FileSystemCacheProvider

• It is traversing the entire file tree every tick.
  There must be a more efficient approach, like, keeping a collection of items that are expiring soon and only checking those. We can populate this collection at startup. Something like what we could do in the MemoryCacheProvider section (see below).

• Why not use Files.getLastModifiedTime directly? To fix The file system cache is not removing files properly #61

• What's the point of sorting paths (.sorted(Comparator.reverseOrder())) if we are not removing empty folders?

• There is no need to convert objects to LocalDateTime: Duration.between accepts Instants or any Temporal for that matter.
  Something like

var diff = Duration.between(
  Files.getLastModifiedTime(path).toInstant(),
  Instant.now()
)

MemoryCacheProvider

• Just use Instant directly

• Instead of using a ticker task that rechecks all entries at a fixed interval, use a smart approach but on top of DelayQueue probably.

// instance properties
private DelayQueue<CacheEntry>expiredEntries;
private Map<Endpoint, CacheEntry> cache;

// In a background thread:
while(true) {
  var x = expiredItems.take();
  // Double-check if it has expired or if it was refreshed (updated). Also, update it's expected expiration timestamp if needed.
  if (x.actuallyExpired()) {
    cache.remove(x);
  } else {
  expiredItems.add(x);
  }
}

• In clearOldCache, why are we sorting? Why even create a list?
  we could just use .removeIf(), .forEach or for (var entry : map.entrySet())
  or an iterator https://stackoverflow.com/a/6092681
  premature optimization? ;-;
  Funny, Spring does exactly this :(
  https://www.tabnine.com/code/java/methods/java.util.Set/removeIf?snippet=5ce7138c7e034400044e634a

MongoDBCacheProvider

• connectionUri is not used. Values are hard coded.

MySQLCacheProvider

• Why not make it JDBCCache? More generic.
  • Or at least support Postgres. It has APIs to work with "large objects" (think: blobs or raw files) and JSON.
    Also, it is open source, more standards compliant than Java, and is supported by all popular cloud providers, inc Vercel, Google, Amazon, and Azure.

General notes

• Ensure that the task is stopped before returning or at least check if the task isCancelled before trying to cancel it.

if (clearTask != null)
{
    clearTask.cancel(false);
// Properly wait for the task to finish. Probably wrap it in try-catch
+    clearTask.get();
+    clearTask = null;
}

• store and update look very similar and redundant. Replace them with put?

• Storing:
  if it's invalid, it will throw and not return null, right? We won't reach this point if an exception is thrown.

// If the object we are trying to store is not valid, don't store it.
if (storeItem == null)

• Writing:
  Files.write creates or overrides the file by default, so there is no reason to redo all of the above before calling writeObject

-try
-{
-    Files.createFile(storePath);
-} catch (FileAlreadyExistsException e)
-{
-    // ignore it
-}
writeObject

-Rename clear methods:

• clear() removes all files that belong to a specific Endpoint without taking expiration into account.
  purge sounds more fitting.

• clearOldCache() almost does what its name suggests. It delegates the actual clearing of paths to clearPath.
  Although, IMO, it shouldn't check if TTL = Infinity because what if we want to call it manually? This behavior is not specified in (and may even conflict with )the interface.
  clearOldCache should not be called anyways. if TTL is infinity, the clearTask will be null.

• clearPath should be renamed to clearOldPath or clearPathIfOld or just clearIfOld (a la Files.deleteIfExists).