Skip to content

fix(os-index): idempotent delete + batched alias lookup in OSIndexAPIImpl (#36501)#36560

Open
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36501-osindexapi-robustness
Open

fix(os-index): idempotent delete + batched alias lookup in OSIndexAPIImpl (#36501)#36560
fabrizzio-dotCMS wants to merge 1 commit into
mainfrom
issue-36501-osindexapi-robustness

Conversation

@fabrizzio-dotCMS

Copy link
Copy Markdown
Member

Proposed Changes

Two OpenSearch index-management robustness gaps that surface under migration phase 2, where OS is read/write primary so its errors propagate. Contributes to #36501.

  • idempotent deletedelete(indexName) threw on any failure, including index_not_found. ES tolerated deleting an absent index; OS must too, else phase-2 cleanup paths fail hard on a missing index. Now treats index_not_found (HTTP 404) as an idempotent no-op; other errors still propagate.
  • batched alias lookupgetIndexAlias(List) put every index name on one _alias GET request line; with a large set (100+ site-search indices) it exceeded OpenSearch's ~4096-byte line limit (too_long_http_line_exception), aborting the whole lookup and reporting a valid alias as missing. Now batches the lookup (ALIAS_LOOKUP_BATCH_SIZE) and merges results.

Verification

Phase 2, isolated env: ESSiteSearchAPITest reindex tests 2/0/0 (delete idempotency); SiteSearchWebAPITest#search_byAlias_resolvesIndex passes. Note: the alias-overflow path needs 100+ indices to trigger the original bug, so the fix is exercised defensively rather than reproduced in isolation.

Refs #36501, #36320.

…Impl (#36501)

Two OpenSearch index-management robustness gaps that surface under migration
phase 2, where OS is the read/write primary so its errors propagate instead of
being swallowed as a shadow copy.

- delete(indexName) threw RuntimeException on any failure, including
  index_not_found. ES tolerated deleting an absent index; OS must too, otherwise
  phase-2 cleanup paths (e.g. ESSiteSearchAPITest reindex teardown) fail hard on a
  missing index. Treat index_not_found (HTTP 404) as an idempotent no-op success;
  all other errors still propagate. (Fixes ESSiteSearchAPITest
  .testFullReindexKeepsDefaultSiteSearchIndex / .testReindexAbortKeepsDefaultSiteSearchIndex
  under phase 2.)

- getIndexAlias(List) put every index name on one _alias GET request line; with a
  large set (100+ site-search indices) the request exceeded OpenSearch's ~4096-byte
  line limit (too_long_http_line_exception), the whole lookup aborted, and a valid
  alias was reported as missing. Batch the lookup (ALIAS_LOOKUP_BATCH_SIZE) and
  merge results. (Fixes SiteSearchWebAPITest.search_byAlias_resolvesIndex under phase 2.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 1m 19s —— View job


Code Review

  • Read the diff and surrounding context in OSIndexAPIImpl.java
  • Analyzed both changes (idempotent delete, batched alias lookup)
  • Checked bugs, error paths, replay safety, conventions

I reviewed both changes against OSIndexAPIImpl.java. The PR is well-reasoned and both fixes are correct for their stated goals.

New Issues

  • 🟡 Medium: OSIndexAPIImpl.java:809-815Batching changed the failure granularity of getIndexAlias, and a mid-loop failure is now silent. Previously a failing lookup produced an empty map (fully aborted). Now if batch N of M throws too_long_http_line_exception or any other error, batches 1..N-1 have already merged into aliases, the loop jumps to catch, and the method returns a partial map that looks complete to the caller — the very "valid alias reported as missing" symptom this PR fixes, just narrowed to the un-processed batches. This is strictly better than the pre-PR behavior (not a regression), but the Logger.warnAndDebug is the only signal that the result is incomplete. Worth confirming callers of getIndexAlias treat an absent entry as "unknown" rather than "confirmed no alias." No code change strictly required.

Notes (non-blocking)

  • isIndexNotFound (:292-299) inspects only e and one level of e.getCause(). The status() == 404 check is applied to e only — if the OpenSearchException is wrapped more than one level deep, that branch misses it. In practice the message-substring fallback (index_not_found / no such index) should still catch it since the root message is checked, so coverage is adequate. String.valueOf(root.getMessage()) correctly avoids an NPE on a null message. Looks fine.
  • ALIAS_LOOKUP_BATCH_SIZE = 50: with long site-search index names (cluster-ID prefix + site + timestamp), 50 names can approach the ~4096-byte line budget. It's under the limit for typical names, and the batching is the right mechanism — just flagging that the constant is a soft margin, not a hard guarantee, if index names grow.

Verdict

No blocking issues. The delete idempotency (treating 404 / index_not_found as a no-op success) is correct and matches ES semantics, and deleteMultiple inherits the fix cleanly. The batched alias lookup with Lists.partition + merge is the right approach.
· issue-36501-osindexapi-robustness

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.

1 participant