Skip to content

Conversation

@szybia
Copy link
Contributor

@szybia szybia commented Aug 13, 2025

No description provided.

return Map.of("count", entryCount(), "memory", ram.toString(), "memory_in_bytes", ram.getBytes());
final Cache.Stats bitsetCacheStats = bitsetCache.stats();

final HashMap<String, Object> stats = new LinkedHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Use Map (the interface) as the type here on the LHS, rather than HashMap (the concrete superclass of LinkedHashMap).

@joegallo
Copy link
Contributor

I realize this is mostly an academic matter, but I'd prefer to split this into two PRs: one that doesn't add time tracking (that is, it only does small bit of additional work that is exposing the things that we're already tracking), and another that adds the time tracking.

private final Map<IndexReader.CacheKey, Set<BitsetCacheKey>> keysByIndex;
private final AtomicLong cacheFullWarningTime;
private final AtomicLong hitsTimeTakenNanos;
private final AtomicLong missesTimeTakenNanos;
Copy link
Contributor

@joegallo joegallo Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://en.wikipedia.org/wiki/Rectification_of_names -- do not express individuality, make your thing like the other things.

joegallo@simulacron:~/Code/elastic/elasticsearch $ git grep -E '(hitsTime|missesTime)' | grep Nanos | grep 'private final'
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java:    private final AtomicLong hitsTimeInNanos = new AtomicLong(0);
modules/ingest-geoip/src/main/java/org/elasticsearch/ingest/geoip/GeoIpCache.java:    private final AtomicLong missesTimeInNanos = new AtomicLong(0);
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichCache.java:    private final AtomicLong hitsTimeInNanos = new AtomicLong(0);
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichCache.java:    private final AtomicLong missesTimeInNanos = new AtomicLong(0);

@joegallo
Copy link
Contributor

joegallo commented Aug 13, 2025

I realize you're doing what the rest of the preexisting code is doing by using an AtomicLong to track the nanos, but I suspect we should be using a CounterMetric (which itself just wraps a LongAdder) instead.

Since that's what the existing code does, then that means there's yet another new PR that I'd like you to put in first which will change those over (or we'll figure out together why that's not feasible).

@szybia
Copy link
Contributor Author

szybia commented Aug 13, 2025

splitting up PRs and taking comments into account 🚀

@szybia szybia closed this Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants