CMR-11070: Fix bug when finalizing rebalancing back to small collections, fix in-memory db to allow rebalancing#2384
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFormatting and local-binding refactors across indexer resharding/rebalancing/reset flows; added a public in-memory helper Changes
Sequence Diagram(s)(Skipped — changes are refactors, a single-component in-memory helper, and a unit test; no multi-component sequential flow to visualize.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@indexer-app/src/cmr/indexer/services/index_set_service.clj`:
- Line 514: Remove the stray debug println call `(println
"~~~~~~~coll-base-name: " coll-base-name)` from index_set_service.clj; if you
need runtime visibility of coll-base-name, replace it with a structured logger
call such as `(log/info ...)` or `(log/debug ...)` (or the project's logging fn)
within the same function/scope where `coll-base-name` is bound so observability
uses the app's logging framework instead of println.
- Around line 552-590: The finalize-collection-rebalancing function may call
es/delete-index with a nil old-separate-index; before calling es/delete-index in
the when (= "small-collections" target) branch, guard that old-separate-index is
non-nil (and optionally non-empty) and only call es/delete-index when it exists;
reference the local old-separate-index (from get-in old-gran-index-set
[:index-set :concepts :granule (keyword concept-id)]) and the es/delete-index
call to locate and protect the delete invocation.
In `@metadata-db-app/src/cmr/metadata_db/data/memory_db.clj`:
- Around line 285-305: The batch finder is sorting by a non-existent :id key
causing non-deterministic ordering; update the sort in find-concepts-in-batches
to use :concept-id instead of :id (i.e., change the sort-by call that produces
sorted-concepts to sort by :concept-id) so batches are deterministic and match
the Oracle behavior, leaving the rest of the lazy-find/batch logic unchanged and
still associating :provider-id onto each returned concept.
🧹 Nitpick comments (1)
metadata-db-app/src/cmr/metadata_db/data/memory_db.clj (1)
699-719: Test/demo code in thecommentblock is fine for REPL use but note it won't work as-is.The test snippet calls
save-conceptwith a provider map as the second argument (line 706), butsave-concept(line 452) expects the second arg to be a provider and then validates concept fields like:revision-id(via the:preassertion). The test concepts also lack:metadataand other fields thatafter-saveor downstream logic may expect. This is minor sincecommentblocks are never evaluated, but it could mislead someone trying to use it at the REPL.
zimzoom
force-pushed
the
CMR-11070
branch
2 times, most recently
from
b492092 to
383c4f4
Compare
|
Adding/testing CodeRabbit suggestions, please hold. |
|
Note that the suite of system int tests for testing rebalancing collections BACK to small collections has been commented out and marked as non-operational as of some time prior to this ticket, which is why that has not been updated here. (larger effort/out of scope) |
| ;; Update the index set. This will create the new collection indexes as needed. | ||
| (validate-requested-index-set context es-config/gran-elastic-name gran-index-set true) | ||
| (update-index-set context es-config/gran-elastic-name (util/remove-nils-empty-maps-seqs gran-index-set)))) | ||
| new-gran-index-set (as-> old-gran-index-set index-set |
There was a problem hiding this comment.
All these lines for defining this var new-gran-index-set actually remain the same, except the names in this first line were updated to have the new- and old- prefixes.
| (if (= "small-collections" target) | ||
| (remove-granule-index-from-index-set index-set concept-id) | ||
| index-set)) | ||
| es-store (indexer-util/context->es-store context es-config/gran-elastic-name)] |
There was a problem hiding this comment.
Added es-store because needed for deleting indices
| es-store (indexer-util/context->es-store context es-config/gran-elastic-name)] | ||
| (try | ||
| ;; Update the index set. This will create the new collection indexes as needed. | ||
| (validate-requested-index-set context es-config/gran-elastic-name new-gran-index-set true) |
There was a problem hiding this comment.
These two forms (validate-requested-index-set and (update-index-set below remain the same, they have just been wrapped in try.
| (update-index-set context es-config/gran-elastic-name (util/remove-nils-empty-maps-seqs new-gran-index-set)) | ||
|
|
||
| ;; Delete the separate index for this collection when moving back into small collections index | ||
| (when (= "small-collections" target) |
There was a problem hiding this comment.
This when is the new logic for deleting
| (println "Batch:") | ||
| (doseq [concept batch] | ||
| (println (select-keys concept [:concept-id :native-id]))) (println)) | ||
| ) |
There was a problem hiding this comment.
My LSP did not like this but I left it because I could have sworn that rich comments are the one time you are supposed to have the ending paren on its own line, for IDE line-wise selection.
There was a problem hiding this comment.
as a lover of:
{
}
I always hated putting ) with all the others, it makes it so hard to figure out how to add/remove things.
what's up with the floating println? I understand your just writing a new line after a loop, but it is just hanging out there
There was a problem hiding this comment.
I am not sure if you are asking about what's up with its existence or up with its formatting, so --
it exists to make everything easier to read when there is many loops,
& its formatting is interesting, that was the LSP autoformatter (which is the official Clojure one). I had it on its own line and it got moved up to same line as previous form with "that number of spaces" buffer. I think it basically detected it was printing nothing and assumed that logically it should go with the previous form for readability? Never seen that before
There was a problem hiding this comment.
I am not bothered so much by putting ) with all the others because putting them on their own line would be way way way too cluttered for Lisps, and because it's expected that the IDE has form-wise selection. But in the case of rich comments I could have sworn it was supposed to be on its own line ... IDK, maybe they changed it but I have a feeling it might be a missing LSP feature. Conceptually it also makes it more readable because rich comments are the one time when you should not interpret the entire form as one thing, some pieces of it might execute, others might need something else to resolve, etc.
There was a problem hiding this comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2384 +/- ##
==========================================
- Coverage 57.93% 57.92% -0.01%
==========================================
Files 1066 1066
Lines 73217 73249 +32
Branches 2120 2119 -1
==========================================
+ Hits 42418 42433 +15
- Misses 28824 28840 +16
- Partials 1975 1976 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| filtered-concepts (concepts/search-with-params concepts (assoc params :provider-id provider-id)) | ||
| sorted-concepts (vec (sort-by :concept-id filtered-concepts)) | ||
| start-index (max requested-start-index 0)] | ||
| (letfn [(find-batch [start-index] |
There was a problem hiding this comment.
did AI suggest letfn, I seam to be seeing it more often these days.
There was a problem hiding this comment.
Yes. I think it's neat and useful
jceaser
left a comment
jceaser
left a comment
There was a problem hiding this comment.
minor questions I feel you can handle.
is it possible to add some smaller unit tests, especially for some of the error cases? |
| (when (< start-index (count sorted-concepts)) | ||
| (let [batch (find-batch start-index)] | ||
| (if (empty? batch) | ||
| nil |
There was a problem hiding this comment.
if blocks with nils should be converted to whens
There was a problem hiding this comment.
🧹 Nitpick comments (2)
indexer-app/test/cmr/indexer/test/services/index_set_service_test.clj (2)
502-509: Third assertion is redundant — consider replacing the two path checks with it.
"returns expected index-set structure"at line 508 is a strict superset of the two individual path assertions above it: if(= expected-index-set result)passes, the sub-path checks trivially pass as well. Either keep only the full-structure assertion (simpler), or drop it and keep only the targeted path assertions (easier to diagnose partial failures). Having all three adds noise without additional coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer-app/test/cmr/indexer/test/services/index_set_service_test.clj` around lines 502 - 509, The two narrow assertions using (get-in result [:index-set :granule :indexes]) and (get-in result [:index-set :concepts :granule]) are redundant given the full equality check (is (= expected-index-set result)); remove the two specific (testing ...) blocks and keep only the full-structure assertion that compares result to expected-index-set so the test is simpler and non-duplicative (referencing symbols: result, expected-index-set, get-in, :index-set :granule :indexes, :index-set :concepts :granule).
486-509: Add edge-case tests: selective removal, absent concept-id, and field preservation.The current test only exercises the happy path with a single-index fixture. Three important cases are uncovered, which is especially relevant given the system integration tests for rebalancing back to small-collections remain commented out:
- Selective removal – when multiple indexes exist in
:granule :indexes, only the target should be removed; verify the remaining indexes are untouched.- Absent concept-id – calling the function when
collection-concept-idis not present in:concepts :granule(verify graceful/no-op behaviour rather than a silent corruption or exception).- Sibling field preservation – when
:granulealso contains:rebalancing-collections/:resharding-indexesetc., those fields must survive the removal unmodified.🧪 Suggested additional test cases
+ (testing "does not remove other indexes when multiple are present" + (let [other-index "1_c9999_other" + multi-index-set {:index-set + {:granule + {:indexes [{:name index-name :number_of_shards 5} + {:name other-index :number_of_shards 3}]} + :concepts + {:granule {(keyword collection-concept-id) index-name + (keyword "C9999-OTHER") other-index}}}} + result (#'svc/remove-granule-index-from-index-set multi-index-set collection-concept-id)] + (is (= [{:name other-index :number_of_shards 3}] + (get-in result [:index-set :granule :indexes]))) + (is (= {(keyword "C9999-OTHER") other-index} + (get-in result [:index-set :concepts :granule]))))) + (testing "preserves unrelated granule fields (e.g. rebalancing-collections)" + (let [index-set-with-extras {:index-set + {:granule + {:indexes [{:name index-name :number_of_shards 5}] + :rebalancing-collections ["C9999-OTHER"]} + :concepts + {:granule {(keyword collection-concept-id) index-name}}}} + result (#'svc/remove-granule-index-from-index-set index-set-with-extras collection-concept-id)] + (is (= ["C9999-OTHER"] + (get-in result [:index-set :granule :rebalancing-collections])))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer-app/test/cmr/indexer/test/services/index_set_service_test.clj` around lines 486 - 509, Add three new tests for svc/remove-granule-index-from-index-set: (1) "selective removal" — craft an initial-index-set where :index-set :granule :indexes contains multiple index maps including the target (index-name) and assert only that index is removed while others remain unchanged; (2) "absent concept-id" — call remove-granule-index-from-index-set with a collection-concept-id not present under :index-set :concepts :granule and assert the result is a no-op (indexes and other concept entries unchanged, no exception); (3) "sibling field preservation" — include extra keys under :index-set :granule (e.g., :rebalancing-collections, :resharding-indexes) and assert those keys and their values are preserved after removal; reference svc/remove-granule-index-from-index-set and use get-in on [:index-set :granule :indexes], [:index-set :concepts :granule], and the sibling keys to make your assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@indexer-app/test/cmr/indexer/test/services/index_set_service_test.clj`:
- Around line 502-509: The two narrow assertions using (get-in result
[:index-set :granule :indexes]) and (get-in result [:index-set :concepts
:granule]) are redundant given the full equality check (is (= expected-index-set
result)); remove the two specific (testing ...) blocks and keep only the
full-structure assertion that compares result to expected-index-set so the test
is simpler and non-duplicative (referencing symbols: result, expected-index-set,
get-in, :index-set :granule :indexes, :index-set :concepts :granule).
- Around line 486-509: Add three new tests for
svc/remove-granule-index-from-index-set: (1) "selective removal" — craft an
initial-index-set where :index-set :granule :indexes contains multiple index
maps including the target (index-name) and assert only that index is removed
while others remain unchanged; (2) "absent concept-id" — call
remove-granule-index-from-index-set with a collection-concept-id not present
under :index-set :concepts :granule and assert the result is a no-op (indexes
and other concept entries unchanged, no exception); (3) "sibling field
preservation" — include extra keys under :index-set :granule (e.g.,
:rebalancing-collections, :resharding-indexes) and assert those keys and their
values are preserved after removal; reference
svc/remove-granule-index-from-index-set and use get-in on [:index-set :granule
:indexes], [:index-set :concepts :granule], and the sibling keys to make your
assertions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
metadata-db-app/src/cmr/metadata_db/data/memory_db.clj (1)
284-303: Replace(not (empty? batch))with(seq batch)and add batch-size validation.Line 301 uses the anti-pattern
(not (empty? batch)), which is non-idiomatic Clojure. Use(seq batch)instead. Additionally, the function lacks a guard against invalidbatch-sizevalues: a zero value causes an infinite loop (batch never advances), and negative values causesubvecto fail. Add a precondition{:pre [(pos? batch-size)]}to enforce this.Suggested changes
- ([db provider params batch-size requested-start-index] + ([db provider params batch-size requested-start-index] + {:pre [(pos? batch-size)]} (let [{:keys [concept-type]} params provider-id (:provider-id provider) concepts @(:concepts-atom db) filtered-concepts (concepts/search-with-params concepts (assoc params :provider-id provider-id)) sorted-concepts (vec (sort-by :concept-id filtered-concepts)) start-index (max requested-start-index 0)] (letfn [(find-batch [start-index] (let [end-index (min (+ start-index batch-size) (count sorted-concepts)) batch (subvec sorted-concepts start-index end-index)] (mapv #(assoc % :provider-id provider-id) batch))) (lazy-find [start-index] (when (< start-index (count sorted-concepts)) - (let [batch (find-batch start-index)] - (when (not (empty? batch)) - (cons batch (lazy-seq (lazy-find (+ start-index batch-size))))))))] + (let [batch (find-batch start-index)] + (when (seq batch) + (cons batch (lazy-seq (lazy-find (+ start-index batch-size))))))))] (lazy-find start-index)))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata-db-app/src/cmr/metadata_db/data/memory_db.clj` around lines 284 - 303, The function find-concepts-in-batches should validate batch-size and use idiomatic emptiness checks: add a precondition {:pre [(pos? batch-size)]} to the find-concepts-in-batches arity that accepts requested-start-index to prevent zero/negative sizes, and in the lazy-find helper replace (not (empty? batch)) with (seq batch); this touches the find-batch / lazy-find logic and ensures batch-size advances and subvec is never called with invalid bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@metadata-db-app/src/cmr/metadata_db/data/memory_db.clj`:
- Around line 284-303: The function find-concepts-in-batches should validate
batch-size and use idiomatic emptiness checks: add a precondition {:pre [(pos?
batch-size)]} to the find-concepts-in-batches arity that accepts
requested-start-index to prevent zero/negative sizes, and in the lazy-find
helper replace (not (empty? batch)) with (seq batch); this touches the
find-batch / lazy-find logic and ensures batch-size advances and subvec is never
called with invalid bounds.
4 participants
Overview
What is the objective?
Fixes a bug where if a granule index is rebalanced back into small collections index, the old separate granule index does not get deleted.
What are the changes?
remove-granule-index-from-index-setfunction to properly remove old index from the index setfinalize-collection-rebalancingfunction to delete the old index when the "target" is small collections index. Although the ticket describes this as occurring when the old granule index was resharded, this was not getting deleted regardless of resharding history or lack thereof, which is why the logic has been added here as a new block.What areas of the application does this impact?
indexer-app
Required Checklist
Additional Checklist
Summary by CodeRabbit
New Features
Tests
Chores