CMR-11271: Fix Cascade collection delete does not delete from index-set when index is separate from small_collections by jaortega527 · Pull Request #2474 · nasa/Common-Metadata-Repository

jaortega527 · 2026-07-30T20:35:33Z

Overview

What is the objective?

Ensure that deleting a collection with a separate granule index also removes the corresponding mapping from the index set, avoiding recreation of deleted indexes and preventing invalid rebalancing attempts.

What are the changes?

Added HTTP status information for individual granule-index deletions in delete-granule-index. Added unit tests to confirm behavior.
Updated cascade-collection-delete to remove the index-set mapping only when ES returns 200 or 404. If the underlying index deletion in ES fails, the index-set is not updated. Added unit tests to confirm behavior.
Added remove-collection-granule-index-if-exists to remove and persist the deletion of the of the separate granule-index mapping. It also handles missing mappings and collections included in small_collections. Added unit tests to confirm behavior.
Added integration test to verify that separate index is deleted when cascade-collection-delete is called
Added integration test to verify that a deleted collection does not rebalance after its separate index mapping is removed

What areas of the application does this impact?

indexer-app
system-int-test

Required Checklist

New and existing unit and int tests pass locally and remotely
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)
My changes generate no new warnings

Additional Checklist

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

codecov-commenter · 2026-07-30T23:39:05Z

Codecov Report

❌ Patch coverage is 4.16667% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.52%. Comparing base (ec7813e) to head (d5b4003).

Files with missing lines	Patch %	Lines
...app/src/cmr/indexer/services/index_set_service.clj	8.33%	11 Missing ⚠️
...xer-app/src/cmr/indexer/services/index_service.clj	0.00%	7 Missing ⚠️
indexer-app/src/cmr/indexer/data/elasticsearch.clj	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2474      +/-   ##
==========================================
- Coverage   27.28%   25.52%   -1.77%     
==========================================
  Files        1013     1013              
  Lines       71208    71226      +18     
  Branches     1139     1047      -92     
==========================================
- Hits        19426    18177    -1249     
- Misses      50645    52002    +1357     
+ Partials     1137     1047      -90

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jmaeng72 · 2026-08-03T17:49:18Z

+          (is (= @validate-arg (:index-set @update-args)))
+          (is (= 33 (:revision-id @update-args))))))))
+
+(deftest delete-index-set-indices-gates-index-set-doc-delete-on-200-status


this wording confuses me

Agreed. These tests weren't intended to be included. The change to the delete-index-set-indices method is not necessary for CMR-11271 and was an artifact of an early attempt to address CMR-11271. the change to delete-index-set-indices has been removed.

jmaeng72 · 2026-08-03T17:49:33Z

+          (is (= 33 (:revision-id @update-args))))))))
+
+(deftest delete-index-set-indices-gates-index-set-doc-delete-on-200-status
+  (testing "deletes index-set document when all index deletes return 200"


what does this mean? What is an index-set document? Only ES has documents

Changes were reverted.

jmaeng72 · 2026-08-03T17:50:23Z

+                    es/delete-index (fn [_ _] {:status 200})
+                    es/delete-document (fn [& _] (reset! delete-document-called? true))]
+        (#'svc/delete-index-set-indices {} 1 es-config/elastic-name)
+        (is (true? @delete-document-called?)))))


are you double checking that the elastid delete-document is called when we delete-index?

changes were reverted

jmaeng72 · 2026-08-03T17:51:41Z

+                    config/idx-cfg-for-index-sets (fn [_] {:index-name "index-sets" :mapping {:index-set {}}})
+                    indexer-util/context->es-store (fn [_ _] {})
+                    es/delete-index (fn [_ index-name]
+                                      (if (= index-name "1_c123_prov")


this test will have one valid and one non-valid deletion? Since 1_c123_prov will return 200 and that's a deletion

changes were reverted

jmaeng72 · 2026-08-03T17:51:48Z

+             java.lang.Exception
+             #"index delete operation failed"
+             (#'svc/delete-index-set-indices {} 1 es-config/elastic-name)))
+        (is (false? @delete-document-called?))))))


new line needed

changes were reverted

jmaeng72 · 2026-08-03T18:03:44Z

+        {:status 200})
+
+      :else
+      (let [updated-gran-index-set (remove-granule-index-from-index-set gran-index-set concept-id)]


if we fall into this case, will it return a status similar to the other cases?

change was reverted

jmaeng72 · 2026-08-03T18:04:49Z

+   - collection has no explicit granule index mapping in the index-set
+   - collection currently maps to the small_collections index"
+  [context concept-id]
+  (let [index-set-id index-set/index-set-id


we have the ability to create index-sets with other id numbers, if so, then this func will not work on them. I suggest we put the index-set-id as a func param to make this more flexible

Updated remove-collection-granule-index-if-exists adding index-set-id as a parameter. When the function is called it uses the index-set-service/index-set-id as the value for the index-set-id.

jmaeng72 · 2026-08-03T18:07:39Z

+        (info (format "No separate granule index mapping found for collection [%s]; skipping index-set cleanup." concept-id))
+        {:status 200})
+
+      (= mapped-index small-collections-index)


I don't think this will ever happen. IF concept-id is truly only a concept-id, it will be c_1234 format, therefore collection-key = :c_1234 and then mapped-index will look for that key

ALSO, the format translation between concept-id and the actual key used in concepts granule map is DIFFERENT and changes based on resharded vs orig index AND orig index and mapped index.

Please look at PROD's index-set to get an example of how these formats are different

Updated remove-collection-granule-index-if-exists to use the collection-ids-from-granule-indexes helper to determine if the collection has a separate index configuration. Also removed the check on whether the mapped-index is the small-collection-index.

jmaeng72 · 2026-08-03T18:10:08Z

-    (dorun (map #(es/delete-index (indexer-util/context->es-store context es-cluster-name) %) index-names))
-    (es/delete-document context index-name idx-mapping-type index-set-id es-cluster-name)))
+        idx-mapping-type (first (keys mapping))
+        delete-responses (keep #(es/delete-index (indexer-util/context->es-store context es-cluster-name) %) index-names)


es/delete-index already check for 200 status and throws an internal-error, why are we doing the check here?

This change to the delete-index-set-indices method is not necessary for CMR-11271 and was an artifact of an early attempt to address CMR-11271. the change to delete-index-set-indices has been removed.

jmaeng72 · 2026-08-03T18:12:53Z

+        (let [resp (es/delete-granule-index context index)
+              status (get resp :status)]
+          (if (contains? #{200 404} status)
+            (vreset! deleted-separate-index? true)


why do we have this new additional step here?

Modified to call remove-collection-granule-index-if-exists immediately after the status of the delete-granule-index call is confirmed to be 200 or 404.

…anule index from index-set if it is a separate index and not in small_cllections

…llections-test to expect the 400 error returned because the deleted index no longer has an entry in the index-set

…letion

…lete success or ES returns a 404

…ttp status in the response

… the existing delete-index-set-indices behavior and remove its associated tests. These changes were unintentionally included and are outside the scope of the collection granule index-set cleanup.

…on-granule-index-if-exists once we confirm the status from the call to delete-granule-index is 200 or 404

jmaeng72 requested changes Aug 3, 2026

View reviewed changes

jaortega527 added 9 commits August 4, 2026 16:31

[CMR-11271] Updated cascade-collection-delete to delete collection gr…

14983b3

…anule index from index-set if it is a separate index and not in small_cllections

CMR-11271 - Updated rebalance-old-deleted-collection-back-to-small-co…

8530a79

…llections-test to expect the 400 error returned because the deleted index no longer has an entry in the index-set

CMR-11271 - Update required to add the gate on successful ES index de…

930772f

…letion

CMR-11271 - Update to delete an index from the index set if the ES de…

72e32ba

…lete success or ES returns a 404

CMR-11271 - Testing a change to delete-granule-index to include the h…

c59b841

…ttp status in the response

CMR-11271 - Added integration test for cascade-collection-delete

2c29266

CMR-11271 - Reverting unrelated index-set deletion changes. Restoring…

f1dbd1a

… the existing delete-index-set-indices behavior and remove its associated tests. These changes were unintentionally included and are outside the scope of the collection granule index-set cleanup.

CMR-11271 - Updated cascade-collection-delete to call remove-collecti…

6e1cd5d

…on-granule-index-if-exists once we confirm the status from the call to delete-granule-index is 200 or 404

CMR-11271 - Updated address PR feedback and clarification

d5b4003

jaortega527 force-pushed the CMR-11271 branch from d20d5e7 to d5b4003 Compare August 4, 2026 21:31

Uh oh!

Conversation

jaortega527 commented Jul 30, 2026

Overview

What is the objective?

What are the changes?

What areas of the application does this impact?

Required Checklist

Additional Checklist

Uh oh!

codecov-commenter commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 30, 2026 •

edited

Loading