CMR-11271: Fix Cascade collection delete does not delete from index-set when index is separate from small_collections - #2474
CMR-11271: Fix Cascade collection delete does not delete from index-set when index is separate from small_collections#2474jaortega527 wants to merge 6 commits into
Conversation
…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
…lete success or ES returns a 404
…ttp status in the response
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2474 +/- ##
==========================================
+ Coverage 57.93% 57.96% +0.03%
==========================================
Files 1073 1073
Lines 74658 74690 +32
Branches 2174 2184 +10
==========================================
+ Hits 43251 43294 +43
+ Misses 29379 29353 -26
- Partials 2028 2043 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| (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 |
| (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" |
There was a problem hiding this comment.
what does this mean? What is an index-set document? Only ES has documents
| 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?))))) |
There was a problem hiding this comment.
are you double checking that the elastid delete-document is called when we delete-index?
| 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") |
There was a problem hiding this comment.
this test will have one valid and one non-valid deletion? Since 1_c123_prov will return 200 and that's a deletion
| java.lang.Exception | ||
| #"index delete operation failed" | ||
| (#'svc/delete-index-set-indices {} 1 es-config/elastic-name))) | ||
| (is (false? @delete-document-called?)))))) No newline at end of file |
| {:status 200}) | ||
|
|
||
| :else | ||
| (let [updated-gran-index-set (remove-granule-index-from-index-set gran-index-set concept-id)] |
There was a problem hiding this comment.
if we fall into this case, will it return a status similar to the other cases?
| - 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 |
There was a problem hiding this comment.
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
| (info (format "No separate granule index mapping found for collection [%s]; skipping index-set cleanup." concept-id)) | ||
| {:status 200}) | ||
|
|
||
| (= mapped-index small-collections-index) |
There was a problem hiding this comment.
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
| (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) |
There was a problem hiding this comment.
es/delete-index already check for 200 status and throws an internal-error, why are we doing the check here?
| (let [resp (es/delete-granule-index context index) | ||
| status (get resp :status)] | ||
| (if (contains? #{200 404} status) | ||
| (vreset! deleted-separate-index? true) |
There was a problem hiding this comment.
why do we have this new additional step here?
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?
cascade-collection-deleteto 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.remove-collection-granule-index-if-existsto remove and persist the deletion of the of the separate granule-index mapping. It also handles missing mappings and collections included insmall_collections. Added unit tests to confirm behavior.What areas of the application does this impact?
Required Checklist
Additional Checklist