Skip to content

fix(opensearch): prevent ES index recreation and spurious reads in Phase 3#35356

Merged
fabrizzio-dotCMS merged 2 commits intomainfrom
fix/phase3-es-index-creation-34934
Apr 17, 2026
Merged

fix(opensearch): prevent ES index recreation and spurious reads in Phase 3#35356
fabrizzio-dotCMS merged 2 commits intomainfrom
fix/phase3-es-index-creation-34934

Conversation

@fabrizzio-dotCMS
Copy link
Copy Markdown
Member

Summary

In Phase 3 (PHASE_3_OPENSEARCH_ONLY) the system was still checking ES index readiness on every checkAndInitializeIndex() cycle, querying the decommissioned ES cluster and recreating ES indices when none existed. This PR adds three Phase 3 guards to stop all unintended ES traffic.

Closes #34934

Changes

ContentletIndexAPIImpl

  • indexReady(): short-circuits to indexReadyOS() when isMigrationComplete() — no longer calls indexReadyES() in Phase 3
  • initIndex(): guards esNeeded with !isMigrationComplete() so bootstrapAndPoint never creates ES indices in Phase 3

BulkProcessorListener

  • No-arg constructor resolves the primary IndexTag from MigrationPhase.current() — Phase 3 reindex logs now correctly show [OS] instead of [ES], and the config gate REINDEX_BULK_LOG_OS_PROVIDER applies instead of the ES one

Testing

  1. Start dotCMS with FEATURE_FLAG_OPEN_SEARCH_PHASE=3
  2. Trigger a content publish — verify no ES index creation logs appear
  3. Verify reindex logs show [OS] prefix
  4. Verify no requests are made to the ES cluster (Kibana / ES logs stay silent)

Breaking Changes

None.

🤖 Generated with Claude Code

…ase 3

In Phase 3 (OS only), indexReady() still required ES indices to be ready,
causing initIndex() to be triggered and recreating ES indices when none
existed. Every checkAndInitializeIndex() cycle also queried the
decommissioned ES cluster via indexReadyES().

- indexReady(): short-circuit to indexReadyOS() when isMigrationComplete()
- initIndex(): guard esNeeded with !isMigrationComplete() to never bootstrap ES in Phase 3
- BulkProcessorListener: resolve primary IndexTag from current phase so
  Phase 3 reindex logs show [OS] instead of [ES] and the correct config
  gate (REINDEX_BULK_LOG_OS_PROVIDER) applies

Closes #34934

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 16, 2026

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


PR Review

  • Read changed files
  • Examine isMigrationComplete() and MigrationPhase logic
  • Review ContentletIndexAPIImpl changes
  • Review BulkProcessorListener changes
  • Post findings

The logic is correct and the short-circuit evaluation works as intended. A few things worth flagging:

1. Stale javadoc on initIndex() (ContentletIndexAPIImpl.java ~L606)

The method's doc still says:

Phases 1/2/3 (fresh install): creates 2 ES + 2 OS indices sharing the same timestamp.

Phase 3 now only creates OS indices. That bullet needs to be split:

Phases 1/2 (fresh install): creates 2 ES + 2 OS indices…
Phase 3 (fresh install): creates 2 OS indices only; ES is decommissioned.

Fix this →

2. Double read of indexReadyOS() in initIndex() (ContentletIndexAPIImpl.java L614 and L620)

indexReady() (called on L614) already calls indexReadyOS() in Phase 3 — then osNeeded (L620) calls it again. The two calls are consistent in practice (we only reach L620 when indexReadyOS() just returned false), but if OS index state changes between the two calls the guard could misfire. Minor, but worth a note.

3. BulkProcessorListener() snapshots phase at construction time

The no-arg constructor reads MigrationPhase.current() once and freezes it in provider. This is fine only because ReindexThread creates a fresh BulkProcessorListener per batch (L225). If that ever changes to a long-lived instance, the [OS]/[ES] tag would silently stale. The assumption is load-bearing but undocumented.

4. No unit tests for Phase 3 guards

indexReady() is @VisibleForTesting. The Phase 3 branches in both indexReady() and initIndex() have no automated coverage — only the manual steps in the PR description. Given these guards prevent a potentially destructive loop (ES index recreation on every checkAndInitializeIndex() cycle), a parametrized test covering Phase 0 / Phase 2 / Phase 3 would be appropriate. Fix this →


The core fix is sound — !isMigrationComplete() correctly short-circuits before indexReadyES() is called in both the esNeeded expression and the indexReady() routing. The BulkProcessorListener tag change is correct given per-batch construction. Items 1 and 4 are the most actionable before merge.

@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 17, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 17, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 17, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS added this pull request to the merge queue Apr 17, 2026
Merged via the queue into main with commit 88a952c Apr 17, 2026
49 checks passed
@fabrizzio-dotCMS fabrizzio-dotCMS deleted the fix/phase3-es-index-creation-34934 branch April 17, 2026 16:12
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.

[TASK] Migrate ContentletIndexAPIImpl

2 participants