concord-server: inject ProcessKeyCache interface instead of implementation to utilize singleton scope

[benbroadaway]

A nosy heap dump reveals we end up with 145 instances of ProcessKeyCache and 70+ instances of ProcessQueueManager.

Some are more used than others, but there's duplicate entries across the instances so there's duplicated lookups (unnecessary db traffic).

Annotating as @Singleton brings that down to 1 each, as one would expect. This should result in fewer calls to the DB and less memory usage by the cached data over all.

[benbroadaway]

This may be a just a piece of a larger oversight. Or I may be missing a point. Should all implementations of PayloadProcessor be singleton as well? They're not particularly expensive to create. But are they intentionally not singleton currently?

[brig]

This may be a just a piece of a larger oversight. Or I may be missing a point. Should all implementations of PayloadProcessor be singleton as well? They're not particularly expensive to create. But are they intentionally not singleton currently?

interesting....

binder.bind(ProcessKeyCache.class).to(com.walmartlabs.concord.server.process.queue.ProcessKeyCache.class).in(SINGLETON);

  /** One instance per {@link Injector}. Also see {@code @}{@link Singleton}. */
  public static final Scope SINGLETON = new SingletonScope();

/**
 * Apply this to implementation classes when you want only one instance (per {@link Injector}) to be
 * reused for all injections for that binding.
 *
 * @author [email protected] (Bob Lee)
 */
@Target({ElementType.TYPE, ElementType.METHOD})
@Retention(RUNTIME)
@ScopeAnnotation
public @interface Singleton {}

with @singleton annotation:

10:55:22.385 [main] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4 10:55:22.385 [main] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 724612647 10:55:22.385 [main] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=1604949791

without:

10:57:45.934 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 1784527308
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 1006259625
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 805520003
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 1977694546
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 797605812
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 541592470
10:57:45.935 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547

10:57:45.936 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ClassLoader: jdk.internal.loader.ClassLoaders$AppClassLoader@64b8f8f4
10:57:45.936 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - ProcessKeyCache created: 1686722530
10:57:45.936 [qtp29401411-56] [INFO ] [] c.w.c.s.p.queue.ProcessKeyCache - Injector id=588356547
....

[brig]

looks like problem here:

concord/server/impl/src/main/java/com/walmartlabs/concord/server/process/queue/ProcessQueueManager.java

Line 59 in bfcadfd

ProcessKeyCache keyCache,

we should not inject implementation.

so fix is:

import com.walmartlabs.concord.server.sdk.ProcessKeyCache;

and there is also: ProcessKeyCacheGaugeModule :)

[brig]

and docs:
https://github.com/google/guice/wiki/JustInTimeBindings

[benbroadaway]

Ahh, unfortunate class name collision.

Also fixes the metric

# after executing 3 processes and restarting 1
$ curl -s http://localhost:8001/metrics | grep process_key_cache_ | grep -ve '^#' && echo
process_key_cache_miss_count 4.0
process_key_cache_hit_count 256.0

[ibodrov]

Minor comment, otherwise LGTM. Needs a better title?

[ibodrov]

What for?

[benbroadaway]

The interface and implementation have the same class name ProcessKeyCache. And the implementation is in the server.queue package, so without importing either class, guice defaults to the implementation--which isn't configured as a singleton. The interface is explicitly bound in singleton scope.