Ingest field configuration helper cache #2614

dtspence · 2024-10-16T17:44:08Z

Implementation of a field helper cache using an LRU map.

The cache can be optionally enabled on the BaseIngestHelper instance and will wrap an existing FieldConfigHelper implementation.
By default the cache is disabled and must be enabled via configuration.

As part of the work, there was also a factory implementation in separate branch which would allow an implementation to be specified by configuration, which may be preferrable.

Closes #2612

ivakegg · 2024-10-21T13:21:14Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/CachedFieldConfigHelper.java

+
+    private static class ResultEntry {
+        private final String fieldName;
+        private final EnumMap<AttributeType,Boolean> resultMap;


Do we not need to limit the size of this cache?

I think the map here will be limited to the size of the AttributeType (which was in the current class - maybe we should change the name). I tried an alternate version which used a switch statement on the enum and individual boolean variables, but it did not seem to impact the timing w/JMH. I will confirm the timing again - and we can also update to have a switch statement and explicit boolean results.

An approach with a switch statement and primitive boolean variables was slightly quicker on second pass. I updated the logic, please let me know if we should evaluate the previous approach.

Discussed additional performance ideas, going to run additional tests.

The LinkedHashMap implementation tested slightly better in benchmarks. Please indicate if we should switch the implementation or restructure the representation.

...ouse/ingest-core/src/test/java/datawave/ingest/data/config/CachingFieldConfigHelperTest.java

jschmidt10 · 2025-01-22T19:25:19Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/CachedFieldConfigHelper.java

+
+    private void debugLogState() {
+        if (resultCache.hasLimitExceeded()) {
+            log.info("Field cache LRU limit exceeded [limit={}, debug={}, size={}, uniq={}]",


Since this feature is "debugLogState" I was expecting this to be debug level logging. Not a deal breaker but I'd vote to make it debug.

I can update - I set the level this way so it would show up in logs without having to also adjust logger/levels.

jschmidt10 · 2025-01-22T19:27:07Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/CachedFieldConfigHelper.java

+        }
+
+        protected boolean removeEldestEntry(Map.Entry<K,V> eldest) {
+            boolean localLimitExceeded = size() > maxSize;


Is size() threadsafe? Wondering if this could return a stale size in a multi-threaded scenario. Not sure how much we care.

I can make a comment and/or refactor - the intent was to have the limitExceeded variable be thread-safe for the debug log output.

EDIT: The refactored the implementation no longer uses a thread for the debug/diagnostic log messages.

jalphonso · 2025-01-22T20:45:01Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/ingest/BaseIngestHelper.java

The setup() method is already too long. There are blocks of logic preceded by a comment explaining what's to come. I know this is a bit out of scope but it would help with readability if it were refactored so that each of those commented blocks turn into meaningfully named methods.

jalphonso · 2025-01-22T20:47:07Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/ingest/BaseIngestHelper.java

@@ -255,10 +259,19 @@ public void setup(Configuration config) {
        // Load the field helper, which takes precedence over the individual field configurations
        final String fieldConfigFile = config.get(this.getType().typeName() + FIELD_CONFIG_FILE);
        if (fieldConfigFile != null) {
-            if (log.isDebugEnabled()) {
-                log.debug("Field config file " + fieldConfigFile + " specified for: " + this.getType().typeName() + FIELD_CONFIG_FILE);
+            final boolean fieldConfigCacheEnabled = config.getBoolean(this.getType().typeName() + FIELD_CONFIG_CACHE_ENABLED, false);


If the cache truly is the better option do we really want it to be feature based?

My preference was to make it feature/opted-into based on the need to select an appropriate cache value and also so the capability could be turned off in the event of unexpected behavior.

That's reasonable. I suggest that once the confidence level is high, we remove the conditional logic.

jalphonso · 2025-01-22T20:47:45Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/ingest/BaseIngestHelper.java

-                log.debug("Field config file " + fieldConfigFile + " specified for: " + this.getType().typeName() + FIELD_CONFIG_FILE);
+            final boolean fieldConfigCacheEnabled = config.getBoolean(this.getType().typeName() + FIELD_CONFIG_CACHE_ENABLED, false);
+            final boolean fieldConfigCacheLimitDebug = config.getBoolean(this.getType().typeName() + FIELD_CONFIG_CACHE_KEY_LIMIT_DEBUG, false);
+            final int fieldConfigCacheLimit = config.getInt(this.getType().typeName() + FIELD_CONFIG_CACHE_KEY_LIMIT, 100);


how was the default of 100 chosen?

I used a value that felt was a reasonable/minimal default and also fit with the sample datasets. I can change it to something different, would a higher value at 1_000 or 10_000 be better?

I don't have a recommendation on size. I was just curious.

jalphonso · 2025-01-22T20:54:38Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/ingest/BaseIngestHelper.java

-            if (log.isDebugEnabled()) {
-                log.debug("Field config file " + fieldConfigFile + " specified for: " + this.getType().typeName() + FIELD_CONFIG_FILE);


Should probably restore this

It was elevated to an info - however, it is logged twice - there is another location within XMLFieldHelper which outputs the file as info. I will update this back to debug.

With the new diagnostic code if it makes better sense to have it as info then I'm ok with it.

jalphonso · 2025-01-22T21:01:20Z

warehouse/ingest-core/src/main/java/datawave/ingest/data/config/CachedFieldConfigHelper.java

+        this.debugLimitsEnabled = debugLimitEnabled;
+        this.debugFieldUnique = new HashSet<>();
+        this.debugFieldComputes = new AtomicLong();
+
+        if (debugLimitEnabled) {
+            this.debugStateExecutor = Executors.newSingleThreadScheduledExecutor(
+            // @formatter:off
+                new ThreadFactoryBuilder()
+                    .setPriority(NORM_PRIORITY)
+                    .setDaemon(true)
+                    .setNameFormat("CachedFieldConfigHelper.DebugState")
+                    .build()
+                // formatter:off
+            );
+            this.debugStateExecutor.scheduleAtFixedRate(this::debugLogState, DEFAULT_DEBUG_STATE_SECS, DEFAULT_DEBUG_STATE_SECS, SECONDS);
+        } else {
+            this.debugStateExecutor = null;
+        }


Should this debug related code be left in?

I can update the name to be something different and/or refactor. The intent was to trace internals to view and help properly size the cache. It was meant to be conditionally enabled, so not to impact normal execution. I evaluated a way to link to Hadoop metrics, but I didn't immediately see a straight forward path to connect. In respect to the name, would generally changing this to an optional view-state semantic be better or is there another convention that would apply better?

EDIT: we could also update tracing/state logic to all be inline (i.e. remove threading), which when enabled would optionally log out the same information. That would simplify the maintenance/logic.

EDIT#2: refactored the logic to no longer have a background thread to log the message.

The diagnostic nomenclature works for me.

dtspence force-pushed the feature/ingest-cache-field-helper branch 3 times, most recently from db63c0c to 3f7d5b6 Compare October 17, 2024 14:10

dtspence marked this pull request as ready for review October 17, 2024 14:10

ivakegg reviewed Oct 21, 2024

View reviewed changes

jschmidt10 reviewed Oct 24, 2024

View reviewed changes

...ouse/ingest-core/src/test/java/datawave/ingest/data/config/CachingFieldConfigHelperTest.java Outdated Show resolved Hide resolved

dtspence force-pushed the feature/ingest-cache-field-helper branch from 3f7d5b6 to 47d1cf3 Compare October 25, 2024 18:45

dtspence marked this pull request as draft October 25, 2024 20:12

dtspence added 7 commits January 8, 2025 17:11

Field configuration helper cache implementation

7b6568a

Refactored unit test and fixed test bug

ce58f04

Refactored implementation to use explicit switch and unit test update

935db13

Use LinkedHashMap as LRU implementation

0c4d168

Rename unit test for main class name

01f12a3

Logging to show if field limit has been exceeded for debugging

0926492

Refactoring and logging debug to run as thread

3d3c425

dtspence force-pushed the feature/ingest-cache-field-helper branch from 8e5b1fb to 038518c Compare January 8, 2025 17:11

Switch cache helper logs to info during setup

c999a78

dtspence force-pushed the feature/ingest-cache-field-helper branch from 038518c to c999a78 Compare January 8, 2025 17:32

dtspence added the Integration Tested label Jan 13, 2025

dtspence marked this pull request as ready for review January 13, 2025 15:47

jschmidt10 previously approved these changes Jan 22, 2025

View reviewed changes

jschmidt10 reviewed Jan 22, 2025

View reviewed changes

jalphonso requested changes Jan 22, 2025

View reviewed changes

dtspence added 2 commits January 22, 2025 22:14

BaseIngestHelper restored logging levels

f2ac332

CachedFieldHelper revised diagnostic and additional unit tests

6e7cbc5

dtspence dismissed jschmidt10’s stale review via 6e7cbc5 January 23, 2025 03:51

CachedConfigHelper log message update around diagnostic

254edbd

dtspence marked this pull request as draft January 23, 2025 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingest field configuration helper cache #2614

Ingest field configuration helper cache #2614

dtspence commented Oct 16, 2024 •

edited

Loading

ivakegg Oct 21, 2024

dtspence Oct 22, 2024

dtspence Oct 25, 2024

dtspence Oct 25, 2024

dtspence Jan 13, 2025

jschmidt10 Jan 22, 2025

dtspence Jan 22, 2025

jschmidt10 Jan 22, 2025

dtspence Jan 22, 2025 •

edited

Loading

jalphonso Jan 22, 2025

jalphonso Jan 22, 2025

dtspence Jan 22, 2025

jalphonso Jan 23, 2025

jalphonso Jan 22, 2025

dtspence Jan 22, 2025

jalphonso Jan 23, 2025

jalphonso Jan 22, 2025

dtspence Jan 22, 2025

jalphonso Jan 23, 2025 •

edited

Loading

jalphonso Jan 22, 2025

dtspence Jan 22, 2025 •

edited

Loading

jalphonso Jan 23, 2025

		if (log.isDebugEnabled()) {
		log.debug("Field config file " + fieldConfigFile + " specified for: " + this.getType().typeName() + FIELD_CONFIG_FILE);

Ingest field configuration helper cache #2614

Are you sure you want to change the base?

Ingest field configuration helper cache #2614

Conversation

dtspence commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtspence Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalphonso Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtspence Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtspence commented Oct 16, 2024 •

edited

Loading

dtspence Jan 22, 2025 •

edited

Loading

jalphonso Jan 23, 2025 •

edited

Loading

dtspence Jan 22, 2025 •

edited

Loading