Skip to content

Conversation

@zimzoom
Copy link
Contributor

@zimzoom zimzoom commented Jan 15, 2026

Overview

What is the objective?

Fix bug that allows calling the start reshard endpoint on an index that no longer exists.

What are the changes?

Added new validation step for checking existence of index/alias earlier in flow, at the API receiving function where it was already doing other validation.

What areas of the application does this impact?

bootstrap-app

Required Checklist

  • [x ] New and existing unit and int tests pass locally and remotely
  • [x ] clj-kondo has been run locally and all errors in changed files are corrected
  • I have commented my code, particularly in hard-to-understand areas
  • I have made changes to the documentation (if necessary)
  • [x ] My changes generate no new warnings

Additional Checklist

  • [x ] I have removed unnecessary/dead code and imports in files I have changed
  • I have cleaned up integration tests by doing one or more of the following:
    • migrated any are2 tests to are3 in files I have changed
    • de-duped, consolidated, removed dead int tests
    • transformed applicable int tests into unit tests
    • reduced number of system state resets by updating fixtures. Ex) (use-fixtures :each (ingest/reset-fixture {})) to be :once instead of :each

Summary by CodeRabbit

  • Bug Fixes
    • Index existence validation now performed during resharding operations.
    • Error messages now include Elasticsearch cluster name for additional context.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

The resharding API now validates that the Elasticsearch index or alias exists before starting the resharding process. A new validation function checks index existence via ES helper utilities and throws a not-found error with cluster details if validation fails.

Changes

Cohort / File(s) Summary
Resharding API Implementation
bootstrap-app/src/cmr/bootstrap/api/resharding.clj
Adds es-helper and es-index dependencies; introduces validate-index-exists function to verify index/alias existence before resharding; integrates validation check into start function
Test Expectations
system-int-test/test/cmr/system_int_test/bootstrap/reshard_index_test.clj
Updates error message expectation to reflect new format that includes cluster name: "Index or alias [name] does not exist in the Elasticsearch cluster [cluster-name]"

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

hacktoberfest-accepted

Suggested reviewers

  • eereiter
  • chris-durbin
  • ygliuvt

Poem

🐰 An index must exist, we now declare,
Before the shards dance through the air,
With Elasticsearch in our checking hand,
We validate the data, safe and grand! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding validation that an index exists before the reshard API processes the request.
Description check ✅ Passed The description addresses the objective and changes but is missing code comments documentation per the template requirements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


🧹 Recent nitpick comments
bootstrap-app/src/cmr/bootstrap/api/resharding.clj (1)

43-55: Validation correctly integrated into the request flow.

The validation is appropriately placed after cluster name validation (needed for connection) and before shard count validation (fail fast pattern).

Consider adding similar validation to get-status and finalize.

I notice that get-status and finalize don't have the same early index validation. While the test file shows they do return 404 for nonexistent indexes (likely handled at the service layer), adding the same API-level validation would provide:

  1. Consistent error message format across all endpoints
  2. Earlier failure without deeper service-layer calls

This is optional since the current service layer already catches these cases with different messages.


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57b33e0 and 847de00.

📒 Files selected for processing (2)
  • bootstrap-app/src/cmr/bootstrap/api/resharding.clj
  • system-int-test/test/cmr/system_int_test/bootstrap/reshard_index_test.clj
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T16:26:11.404Z
Learnt from: indiejames
Repo: nasa/Common-Metadata-Repository PR: 2328
File: indexer-app/src/cmr/indexer/services/index_set_service.clj:337-349
Timestamp: 2025-10-20T16:26:11.404Z
Learning: In indexer-app/src/cmr/indexer/services/index_set_service.clj, the remove-resharding-index function intentionally returns an empty set when resharding-indexes is nil, rather than throwing an error. This differs from remove-rebalancing-collection and reflects a design preference to avoid nil returns.

Applied to files:

  • system-int-test/test/cmr/system_int_test/bootstrap/reshard_index_test.clj
  • bootstrap-app/src/cmr/bootstrap/api/resharding.clj
🔇 Additional comments (3)
bootstrap-app/src/cmr/bootstrap/api/resharding.clj (2)

8-10: LGTM!

The new namespace imports are appropriate for the added validation functionality.


32-41: LGTM!

The validation correctly checks for both index and alias existence, which is appropriate since the reshard operation could target either. The error message is descriptive, including both the target name and the cluster context.

system-int-test/test/cmr/system_int_test/bootstrap/reshard_index_test.clj (1)

63-66: LGTM!

The test correctly validates the new error message format that includes both the index/alias name and the Elasticsearch cluster name, matching the implementation in validate-index-exists.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jceaser jceaser self-requested a review January 15, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants