-
Notifications
You must be signed in to change notification settings - Fork 101
CMR-11039: reshard API validates that index exists #2372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe resharding API now validates that the Elasticsearch index or alias exists before starting resharding. A new private function obtains an ES connection and checks index existence, returning a not-found service error that includes the Elasticsearch cluster name when the index is missing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Resharding API
participant ESIdx as es-index (conn)
participant ES as Elasticsearch
Client->>API: POST /reshard/start (index)
API->>API: validate-index-exists(context, index, es-cluster-name)
API->>ESIdx: context->conn(context, es-cluster-name)
ESIdx->>ES: exists?(index or alias)
ES-->>ESIdx: exists? result (true/false)
ESIdx-->>API: connection + exists? result
alt index exists
API->>API: proceed with existing validations and start resharding
API-->>Client: 200/started
else index missing
API-->>Client: 404 "Index or alias [name] does not exist in the Elasticsearch cluster [cluster-name]"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 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 |
| (when (string/blank? es-cluster-name) | ||
| (errors/throw-service-error :bad-request "Empty elastic cluster name is not allowed."))) | ||
|
|
||
| (defn- validate-index-exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc string
| [context index es-cluster-name] | ||
| (let [conn (es-index/context->conn context es-cluster-name)] | ||
| (when-not (or (es-helper/exists? conn index) | ||
| (es-helper/alias-exists? conn index)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we allow resharding based on alias names. Can you check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I tested it and it does not allow aliases, it returns index doesn't exist error. I will remove the alias check to avoid confusion
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2372 +/- ##
===========================================
- Coverage 58.01% 29.13% -28.88%
===========================================
Files 1063 1003 -60
Lines 72765 69349 -3416
Branches 2113 1196 -917
===========================================
- Hits 42215 20207 -22008
- Misses 28580 48007 +19427
+ Partials 1970 1135 -835 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
Additional Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.