feat(search): apply per-entity ES mapping limits to existing indices - #19377
feat(search): apply per-entity ES mapping limits to existing indices#19377rob-1019 wants to merge 1 commit into
Conversation
Adds elasticsearch.index.entityMappingLimits.<entity>.<limit> configuration. The configured value is baked into index settings at creation/reindex time and idempotently re-applied to live existing indices on every system-update run. Currently honors totalFields -> index.mapping.total_fields.limit, with a reserved 'default' entity key as a fallback for unlisted entity indices. This replaces the workaround of hand-issuing PUT /<index>/_settings for deployments approaching the default 1000-field cap (typically driven by many structured properties on datasetindex_v2) — manual settings do not survive index rebuilds, this configuration does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
docs
PR SummaryOverview Config binds as Reviewed by Cursor Bugbot for commit e77caea. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e77caea. Configure here.
| // Bootstrap-time Spring wiring — no per-request OperationContext is obtainable here. | ||
| byIndex.put( | ||
| indexConvention.getEntityIndexName(OperationFingerprint.EMPTY, entity), esSettings); | ||
| } |
There was a problem hiding this comment.
Per-entity limits miss semantic indices
High Severity
resolveEntityMappingLimits only registers the V2 entity index via getEntityIndexName, so an explicit entity entry such as dataset never matches the semantic twin (*index_v2_semantic). Those indices reuse full V2 mappings including structured properties and still go through buildIndex, so the documented per-entity limit leaves semantic indices at the default field cap and the same mapping-limit failures persist there.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e77caea. Configure here.
| } catch (Exception e) { | ||
| log.warn( | ||
| "Index: {} - Failed to apply entityMappingLimits {}. Continuing.", indexName, changes, e); | ||
| } |
There was a problem hiding this comment.
Mapping limit apply failures swallowed
High Severity
Failed updateIndexSettings calls for entityMappingLimits are caught, logged as a warning, and ignored so buildIndex still succeeds. System update then reports success while the configured field ceiling was never applied, leaving structured-property indexing broken. This violates the project error-propagation rule that write-path failures must not be best-effort when callers cannot observe them.
Triggered by project rule: DataHub Bugbot rules
Reviewed by Cursor Bugbot for commit e77caea. Configure here.
| @Nonnull | ||
| public Map<String, String> forIndex(@Nonnull String indexName) { | ||
| return byIndex.getOrDefault(indexName, defaults); | ||
| } |
There was a problem hiding this comment.
Default limit hits non-entity indices
Medium Severity
forIndex returns the reserved default map for any index name absent from byIndex. Because buildIndex also runs for graph_service_v1, system-metadata, semantic, and v3 indices, a default total-fields setting is pushed onto those non-entity indices every system update, contrary to the stated “unlisted entities” fallback and able to override v3’s separate maxFieldsLimit.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e77caea. Configure here.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java">
<violation number="1" location="metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java:142">
P2: When an `IndexPrefixResolver` derives prefixes per operation, explicit entity limits are resolved only for the `EMPTY` prefix and are not applied to runtime-prefixed indices. Resolve the configured entity limit against the actual operation-specific index name, or retain entity-keyed limits and resolve them during index building.</violation>
<violation number="2" location="metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java:142">
P1: Resolve explicit entity limits for both the primary and semantic index names. `getEntityIndexName` stores only the V2 name, so `*_index_v2_semantic` falls back to the default and can still hit the 1000-field limit.</violation>
</file>
<file name="metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java">
<violation number="1" location="metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java:439">
P2: Custom agent: **Enforce Pragmatic Test Coverage**
The creation and reindex target-setting path is untested: `entityMappingLimits.forIndex(indexName)` feeds both paths, but the tests only verify live updates on existing indices. Add assertions that a configured `mapping.total_fields.limit` appears in `targetSettings` for a new index and a rebuild.</violation>
<violation number="2" location="metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java:560">
P2: When `DEFAULT` is configured, this applies the entity fallback to graph, system-metadata, and timeseries indices as well as entity indices. Restrict the lookup and live update to entity index types before applying the default.</violation>
<violation number="3" location="metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java:669">
P2: When Elasticsearch rejects or does not acknowledge the settings update, this method continues as if the limit applied. Propagate or retry the failure, and treat an unacknowledged response as unsuccessful so operators can remediate it.</violation>
</file>
<file name="metadata-io/src/test/java/com/linkedin/metadata/search/indexbuilder/ESIndexBuilderTest.java">
<violation number="1" location="metadata-io/src/test/java/com/linkedin/metadata/search/indexbuilder/ESIndexBuilderTest.java:2329">
P2: Custom agent: **Enforce Pragmatic Test Coverage**
The new existing-index entityMappingLimits tests (testBuildIndex_AppliesEntityMappingLimitsToExistingIndex, testBuildIndex_NoPutWhenLimitAlreadyMatches, testBuildIndex_DefaultsFallbackAppliesToUnlistedIndex) only exercise success paths. The two failure behaviors added in applyEntityMappingLimitsToExistingIndex are untested: (1) getIndexSettings throwing an IOException/timeout, which propagates out of buildIndex and can abort a system-update run, and (2) updateIndexSettings throwing, which is caught, logged, and swallowed so the limit is silently never applied. Both are core outcomes of this PR's stated idempotent, failure-tolerant behavior, and a regression in either would pass all three new tests. Add coverage for these failure paths so the main success and failure paths of this new mutation are verified.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } else { | ||
| // Bootstrap-time Spring wiring — no per-request OperationContext is obtainable here. | ||
| byIndex.put( | ||
| indexConvention.getEntityIndexName(OperationFingerprint.EMPTY, entity), esSettings); |
There was a problem hiding this comment.
P1: Resolve explicit entity limits for both the primary and semantic index names. getEntityIndexName stores only the V2 name, so *_index_v2_semantic falls back to the default and can still hit the 1000-field limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java, line 142:
<comment>Resolve explicit entity limits for both the primary and semantic index names. `getEntityIndexName` stores only the V2 name, so `*_index_v2_semantic` falls back to the default and can still hit the 1000-field limit.</comment>
<file context>
@@ -90,14 +95,87 @@ protected Map<String, Map<String, String>> getIndexSettingsOverrides(
+ } else {
+ // Bootstrap-time Spring wiring — no per-request OperationContext is obtainable here.
+ byIndex.put(
+ indexConvention.getEntityIndexName(OperationFingerprint.EMPTY, entity), esSettings);
+ }
+ }
</file context>
| } else { | ||
| // Bootstrap-time Spring wiring — no per-request OperationContext is obtainable here. | ||
| byIndex.put( | ||
| indexConvention.getEntityIndexName(OperationFingerprint.EMPTY, entity), esSettings); |
There was a problem hiding this comment.
P2: When an IndexPrefixResolver derives prefixes per operation, explicit entity limits are resolved only for the EMPTY prefix and are not applied to runtime-prefixed indices. Resolve the configured entity limit against the actual operation-specific index name, or retain entity-keyed limits and resolve them during index building.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java, line 142:
<comment>When an `IndexPrefixResolver` derives prefixes per operation, explicit entity limits are resolved only for the `EMPTY` prefix and are not applied to runtime-prefixed indices. Resolve the configured entity limit against the actual operation-specific index name, or retain entity-keyed limits and resolve them during index building.</comment>
<file context>
@@ -90,14 +95,87 @@ protected Map<String, Map<String, String>> getIndexSettingsOverrides(
+ } else {
+ // Bootstrap-time Spring wiring — no per-request OperationContext is obtainable here.
+ byIndex.put(
+ indexConvention.getEntityIndexName(OperationFingerprint.EMPTY, entity), esSettings);
+ }
+ }
</file context>
| changes.size(), | ||
| changes, | ||
| ack); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
P2: When Elasticsearch rejects or does not acknowledge the settings update, this method continues as if the limit applied. Propagate or retry the failure, and treat an unacknowledged response as unsuccessful so operators can remediate it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java, line 669:
<comment>When Elasticsearch rejects or does not acknowledge the settings update, this method continues as if the limit applied. Propagate or retry the failure, and treat an unacknowledged response as unsuccessful so operators can remediate it.</comment>
<file context>
@@ -573,6 +607,71 @@ public ReindexResult buildIndex(@Nonnull OperationContext opContext, ReindexConf
+ changes.size(),
+ changes,
+ ack);
+ } catch (Exception e) {
+ log.warn(
+ "Index: {} - Failed to apply entityMappingLimits {}. Continuing.", indexName, changes, e);
</file context>
| // Apply configured entityMappingLimits to the live index. Runs independent of the | ||
| // mapping/settings diff below so that limit changes (e.g. mapping.total_fields.limit) take | ||
| // effect on a long-lived index without requiring a reindex. | ||
| applyEntityMappingLimitsToExistingIndex(opContext, indexState.name()); |
There was a problem hiding this comment.
P2: When DEFAULT is configured, this applies the entity fallback to graph, system-metadata, and timeseries indices as well as entity indices. Restrict the lookup and live update to entity index types before applying the default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java, line 560:
<comment>When `DEFAULT` is configured, this applies the entity fallback to graph, system-metadata, and timeseries indices as well as entity indices. Restrict the lookup and live update to entity index types before applying the default.</comment>
<file context>
@@ -525,6 +553,12 @@ public ReindexResult buildIndex(@Nonnull OperationContext opContext, ReindexConf
+ // Apply configured entityMappingLimits to the live index. Runs independent of the
+ // mapping/settings diff below so that limit changes (e.g. mapping.total_fields.limit) take
+ // effect on a long-lived index without requiring a reindex.
+ applyEntityMappingLimitsToExistingIndex(opContext, indexState.name());
+
log.info("Current mappings for index {}", indexState.name());
</file context>
| baseSettings.putAll(indexSettingOverrides.getOrDefault(indexName, Map.of())); | ||
| // entityMappingLimits feeds the same settings on creation/reindex so a new or rebuilt index | ||
| // is born at the configured ceiling, matching the value pushed to live indices below. | ||
| baseSettings.putAll(entityMappingLimits.forIndex(indexName)); |
There was a problem hiding this comment.
P2: Custom agent: Enforce Pragmatic Test Coverage
The creation and reindex target-setting path is untested: entityMappingLimits.forIndex(indexName) feeds both paths, but the tests only verify live updates on existing indices. Add assertions that a configured mapping.total_fields.limit appears in targetSettings for a new index and a rebuild.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-io/src/main/java/com/linkedin/metadata/search/elasticsearch/indexbuilder/ESIndexBuilder.java, line 439:
<comment>The creation and reindex target-setting path is untested: `entityMappingLimits.forIndex(indexName)` feeds both paths, but the tests only verify live updates on existing indices. Add assertions that a configured `mapping.total_fields.limit` appears in `targetSettings` for a new index and a rebuild.</comment>
<file context>
@@ -409,6 +434,9 @@ public ReindexConfig buildReindexState(
baseSettings.putAll(indexSettingOverrides.getOrDefault(indexName, Map.of()));
+ // entityMappingLimits feeds the same settings on creation/reindex so a new or rebuilt index
+ // is born at the configured ceiling, matching the value pushed to live indices below.
+ baseSettings.putAll(entityMappingLimits.forIndex(indexName));
Map<String, Object> targetSetting = ImmutableMap.of("index", baseSettings);
builder.targetSettings(targetSetting);
</file context>
| Assert.assertFalse(result.skippedEmpty()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
P2: Custom agent: Enforce Pragmatic Test Coverage
The new existing-index entityMappingLimits tests (testBuildIndex_AppliesEntityMappingLimitsToExistingIndex, testBuildIndex_NoPutWhenLimitAlreadyMatches, testBuildIndex_DefaultsFallbackAppliesToUnlistedIndex) only exercise success paths. The two failure behaviors added in applyEntityMappingLimitsToExistingIndex are untested: (1) getIndexSettings throwing an IOException/timeout, which propagates out of buildIndex and can abort a system-update run, and (2) updateIndexSettings throwing, which is caught, logged, and swallowed so the limit is silently never applied. Both are core outcomes of this PR's stated idempotent, failure-tolerant behavior, and a regression in either would pass all three new tests. Add coverage for these failure paths so the main success and failure paths of this new mutation are verified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At metadata-io/src/test/java/com/linkedin/metadata/search/indexbuilder/ESIndexBuilderTest.java, line 2329:
<comment>The new existing-index entityMappingLimits tests (testBuildIndex_AppliesEntityMappingLimitsToExistingIndex, testBuildIndex_NoPutWhenLimitAlreadyMatches, testBuildIndex_DefaultsFallbackAppliesToUnlistedIndex) only exercise success paths. The two failure behaviors added in applyEntityMappingLimitsToExistingIndex are untested: (1) getIndexSettings throwing an IOException/timeout, which propagates out of buildIndex and can abort a system-update run, and (2) updateIndexSettings throwing, which is caught, logged, and swallowed so the limit is silently never applied. Both are core outcomes of this PR's stated idempotent, failure-tolerant behavior, and a regression in either would pass all three new tests. Add coverage for these failure paths so the main success and failure paths of this new mutation are verified.</comment>
<file context>
@@ -2325,4 +2325,130 @@ void testBuildIndexIncrementalSubmitsReindex() throws Throwable {
Assert.assertFalse(result.skippedEmpty());
}
+
+ @Test
+ void testBuildIndex_AppliesEntityMappingLimitsToExistingIndex() throws IOException {
+ // Existing index whose current total_fields.limit differs from the configured value.
</file context>
maggiehays
added
the
needs-review
2 participants
Summary
Adds
ELASTICSEARCH_INDEX_ENTITYMAPPINGLIMITS_<ENTITY>_TOTALFIELDS(
elasticsearch.index.entityMappingLimits.<entity>.totalFields) — a batteries-included way toset an entity index's
mapping.total_fields.limitfor operators who don't have direct access tothe Elasticsearch API. The value is applied at index creation/reindex and, idempotently, to
already-existing indices on every system-update run.
Problem
Each structured property adds a mapped field, so users with more than ~1,000 structured
properties exceed the default mapping cap on
datasetindex_v2. The existingELASTICSEARCH_INDEX_BUILDER_ENTITY_SETTINGS_OVERRIDESoption only applies at index creation,so an existing index never gets updated — the only remedy today is a hand-issued
PUT /<index>/_settings, which requires ES API access and silently reverts on the next rebuild.Solution
EntityMappingLimitsconfig: per-entitytotalFields, with a reserveddefaultentityas a fallback for unlisted indices. Unknown limit keys are dropped with a warning.
ESIndexBuilderbakes the limit into settings at creation/reindex; for existing indices itPUTs only values that differ, logging each change as
from -> to.environment-vars.mdrow + a creation-only clarification on the existingoverrides row.
Test plan
ESIndexBuilderTest— applied to an existing index, no-op when already matching,defaultfallbackEntityMappingLimitsResolverTest— resolution, case-insensitive keys, unknown-key dropELASTICSEARCH_INDEX_ENTITYMAPPINGLIMITS_DATASET_TOTALFIELDS=2500,verify
GET datasetindex_v2/_settingson fresh and pre-existing indicesChecklist
🤖 Generated with Claude Code
Summary by cubic
Applies per-entity Elasticsearch mapping limits to both new and existing indices so operators can raise
mapping.total_fields.limitwithout manual ES calls. Previously, limits set via entity settings overrides applied only at index creation; existing indices required manualPUT _settingsthat reverted on rebuild.elasticsearch.index.entityMappingLimits.<entity>.totalFields(env:ELASTICSEARCH_INDEX_ENTITYMAPPINGLIMITS_<ENTITY>_TOTALFIELDS), with reserveddefaultfallback; keys are case-insensitive and unknown limit keys are ignored with a warning.ESIndexBuilderincludes limits at creation/reindex and idempotently updates live indices each system-update run when values differ, logging changes as from -> to; no reindex required.IndexConventionand builds an immutableEntityMappingLimitsused byESIndexBuilder; docs clarifyELASTICSEARCH_INDEX_BUILDER_ENTITY_SETTINGS_OVERRIDESis creation-only.mapping.total_fields.limitviaELASTICSEARCH_INDEX_BUILDER_ENTITY_SETTINGS_OVERRIDESor manual ES calls, setELASTICSEARCH_INDEX_ENTITYMAPPINGLIMITS_DATASET_TOTALFIELDS=<value>(and other entities as needed) to persist across rebuilds.Written for commit e77caea. Summary will update on new commits.