Skip to content

task(ContentletIndexAPIImpl Abstraction layers) Refs: #34938#35103

Merged
wezell merged 3 commits intomainfrom
issue-34938-ESMappingAPI-take2
Mar 25, 2026
Merged

task(ContentletIndexAPIImpl Abstraction layers) Refs: #34938#35103
wezell merged 3 commits intomainfrom
issue-34938-ESMappingAPI-take2

Conversation

@fabrizzio-dotCMS
Copy link
Member

@fabrizzio-dotCMS fabrizzio-dotCMS commented Mar 24, 2026

Summary

Introduces the vendor-neutral write-side abstraction layer for the ES → OpenSearch migration. Extracts all bulk-write and mapping operations from ContentletIndexAPIImpl into provider-specific implementations behind a generic PhaseRouter<T>, completing the dual-write infrastructure needed for migration phases 1–3.

Changes

New abstractions (vendor-neutral contracts)

  • ContentletIndexOperations — write-side interface: bulk requests, bulk processor, index lifecycle, delete-by-content-type
  • IndexBulkRequest, IndexBulkProcessor, IndexBulkListener, IndexBulkItemResult — DTOs replacing ES/OS vendor types in public APIs
  • IndexMappingRestOperations — mapping REST contract (putMapping, getMapping, getFieldMappingAsMap)
  • PhaseRouter<T> — generic, phase-aware dual-write router with read(), write(), writeBoolean(), writeReturning() and checked-exception variants

Elasticsearch implementation

  • ContentletIndexOperationsES — wraps BulkRequest/BulkProcessor behind ContentletIndexOperations
  • MappingOperationsES — wraps ES RestHighLevelClient mapping calls behind IndexMappingRestOperations

OpenSearch implementation

  • ContentletIndexOperationsOSopensearch-java 3.x bulk write implementation with manual OSIndexBulkProcessor (no built-in BulkProcessor in that SDK)
  • MappingOperationsOS — OpenSearch mapping REST calls

Minor changes

  • ContentMappingAPI — expanded with putMapping, getMapping, toJson, toMap, toJsonString methods
  • @IndexRouter.access — changed from single enum value to array ({IndexAccess.READ}, {IndexAccess.READ, IndexAccess.WRITE})
  • IndexConfigHelper — added isMigrationStarted() convenience predicate

Testing

  • ContentletIndexOperationsOSIntegrationTest (480 lines) added to OpenSearchUpgradeSuite
  • Tests cover: index creation, bulk index/delete, bulk processor flush, delete-by-content-type, document count
  • To run locally: ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=ContentletIndexOperationsOSIntegrationTest

This PR fixes: #34938

This PR fixes: #34938

@claude
Copy link
Contributor

claude bot commented Mar 24, 2026

Claude finished @fabrizzio-dotCMS's task in 2m 58s —— View job


PR Review

  • Read key changed files
  • Analyze PhaseRouter design
  • Analyze ContentletIndexOperationsOS
  • Analyze domain objects and abstractions
  • Analyze ContentMappingAPI changes
  • Post review findings

Issues Found

1. Partial dual-write on first-provider failurePhaseRouter lines 150–154, 163–168, 210–214

All write methods iterate providers in order and call sequentially. If ES throws, OS never executes (or vice versa in phase 3 rollback scenarios). During phases 1/2 this silently diverges the two indexes with no compensation or rollback:

public void writeChecked(final ThrowingConsumer<T> action) throws Exception {
    for (final T impl : writeProviders()) {
        action.accept(impl);  // exception here skips remaining providers
    }
}

Same applies to write(), writeBoolean(), and the dual-write path in writeReturning(). The second provider should still execute even if the first fails (best-effort with aggregate error reporting). This is the most operationally risky design flaw in the PR — a high-volume reindex hitting a transient ES error during phase 1 would leave OS permanently behind. Fix this →


2. removeContentFromIndexByContentTypeQueryStringQuery vs matchQueryContentletIndexOperationsOS.java:357–362

ES uses matchQuery("contenttype", structureName), OS uses a concatenated QueryStringQuery string:

QueryStringQuery.of(qs -> qs.query("contenttype:" + structureName.toLowerCase()))

query_string parses Lucene syntax. Content type variable names with characters like -, +, *, /, : will either fail or produce wrong query semantics. The ES version uses a term-level match which is correct and safe. Use a term query in the OS implementation: .term(t -> t.field("contenttype").value(structureName.toLowerCase())). Fix this →


3. createContentIndex spin-loop on an immutable return valueContentletIndexOperationsOS.java:214–219

final CreateIndexStatus status = osIndexAPI.createIndex(indexName, settings, shards);
int i = 0;
while (!status.acknowledged()) {   // status is immutable; never changes
    DateUtil.sleep(100);
    if (i++ > 300) {
        throw new IOException("OS index creation timed out for: " + indexName);
    }
}

If acknowledged() is already true (normal case), the loop never runs — fine. If it's false, the loop burns 30+ seconds polling a value that will never flip. This is also present in ContentletIndexOperationsES so it's an inherited pattern, but it's still broken. Consider asserting or throwing immediately on unacknowledged status rather than spin-waiting on a sealed object.


4. OSIndexBulkProcessor — synchronous, no backoff/retryContentletIndexOperationsOS.java:128–195

The ES BulkProcessor is asynchronous with BackoffPolicy.constantBackoff() and configurable setConcurrentRequests(). The manual OS implementation is fully synchronous and has no retry on transient failure. Under a large reindex with occasional HTTP timeouts, all failures are terminal. This is acknowledged as a known gap, but worth tracking explicitly since behavior diverges in production. No backoff = no resilience during the migration window when OS is under heavier load.


5. MappingOperationsOS — plain new ObjectMapper()MappingOperationsOS.java:46

private static final ObjectMapper MAPPER = new ObjectMapper();  // line 46

ContentletIndexOperationsOS uses DotObjectMapperProvider.createDefaultMapper() (which has custom serializers registered). MappingOperationsOS uses a bare ObjectMapper with default configuration. If getFieldMappingAsMap is called and the mapping JSON contains types that need custom deserializers, behavior will differ. Use DotObjectMapperProvider.createDefaultMapper() for consistency.


6. putToIndex error logging drops item detailsContentletIndexOperationsOS.java:283–287

if (response.errors()) {
    Logger.error(this,
            "OS bulk putToIndex: errors in response for "
                    + osReq.operations.size() + " operations");
}

ES version (ContentletIndexOperationsES.java:199–203) logs response.buildFailureMessage() which includes per-item failure reasons. The OS version logs only the count. Failures become invisible in logs when debugging reindex issues.


7. os-content-mapping.json — duplicate textmapping template name

dynamic_templates array has two objects keyed "textmapping": one at line 17 (matches *_texttext type) and one at line 104 (matches any stringmy_analyzer text type). The second silently shadows the first in template-name lookups in some tooling. The names should be distinct even though they are array entries, not map keys. Fix this →


8. createContentIndex — mapping file load not guardedContentletIndexOperationsOS.java:211

Settings failure is caught and logged; the mapping load is not:

try {
    settings = JsonUtil.getJsonFileContentAsString(OS_SETTINGS_FILE);
} catch (Exception e) { Logger.error(...); }   // guarded

final String mapping = JsonUtil.getJsonFileContentAsString(CONTENT_MAPPING_FILE);  // unguarded

If os-content-mapping.json is absent from the classpath, this throws unchecked and the full context is lost. At minimum it should produce a clear IOException with the filename.

@fabrizzio-dotCMS fabrizzio-dotCMS changed the title #34938 Smaller Sub PR. task(ContentletIndexAPIImpl Abstraction layers) Refs: #34938 Mar 24, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS marked this pull request as ready for review March 24, 2026 17:25
wezell
wezell previously approved these changes Mar 24, 2026
Copy link
Member

@wezell wezell left a comment

Choose a reason for hiding this comment

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

Looks great. I added nit-picking comments that you can respond to or not depending on how you feel. Good stuff.

@fabrizzio-dotCMS fabrizzio-dotCMS requested a review from wezell March 25, 2026 15:02
Copy link
Member

@wezell wezell left a comment

Choose a reason for hiding this comment

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

Good. Thanks Fabro

@wezell wezell enabled auto-merge March 25, 2026 15:07
@wezell wezell added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 336892b Mar 25, 2026
51 checks passed
@wezell wezell deleted the issue-34938-ESMappingAPI-take2 branch March 25, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants