-
Notifications
You must be signed in to change notification settings - Fork 27
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
[Fetch Migration] Migrate index templates and component templates as a part of metadata migration #477
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #477 +/- ##
============================================
+ Coverage 73.51% 73.59% +0.08%
- Complexity 1180 1183 +3
============================================
Files 124 124
Lines 4886 4886
Branches 439 439
============================================
+ Hits 3592 3596 +4
+ Misses 999 997 -2
+ Partials 295 293 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Could you add to the comments or PR description how one would support only metadata migration?
__name: str | ||
__template_def: Optional[dict] | ||
|
||
def __init__(self, template_payload: dict, template_key: str = DEFAULT_TEMPLATE_KEY): |
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.
When would/could there be a different template_key?
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.
IndexTemplateInfo
is a subclass of this class and overrides the template_key
value:
This was done because the structure of the index-template and component-template responses are largely identical except for the difference in template key string.
import requests | ||
|
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.
(on the filename, not this line). Index is both a noun and a verb, so it isn't clear to me whether this applies to operations on indices (nouns) or if this is a set of operations of a particular type (verb). index_management.py, or something that could shake the ambiguity would probably help new contributors.
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.
Point taken. Note that I'd like to do this in a follow-up PR to keep the diff in this one focused on the template migration changes
|
||
def __fetch_templates(endpoint: EndpointInfo, path: str, root_key: str, factory) -> set: | ||
url: str = endpoint.add_path(path) | ||
# raises RuntimeError in case of any request errors |
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.
Can you scope this to something more specific to disambiguate errors up the callstack? Leaving them as ConnectionError, HTTPError, etc or preserving the exception (rather than stringifying) would be preferable. SonarQube has some rules to not use RuntimeExceptions for Java & I the principles apply for other languages.
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.
What happens if there's an exception? If looks like it propagates up? It might be fair that this code doesn't handle it, but if there are compound instructions with dependencies, it will be harder to guarantee success without a more granular retry strategy. Saying that everything executes in < 5s and that these calls have never been observed to fail are probably valid reasons NOT to invest time in making them granular though.
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.
Thanks for this comment. I just learned that Python3 supports exception chaining so i'll incorporate that here instead of stringifying the underlying exception. Note that i'll continue to use RuntimeError
since it's the only built-in exception type that's applicable here. I agree that exception typing in Fetch could be more specific - if you believe that's valuable, I can pick this up as a follow-up refactoring/improvement task.
What happens if there's an exception? If looks like it propagates up?
That's correct. This was an intentional decision to fail the metadata migration if templates could not be fetched (and therefore migrated)
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.
Exception chaining added with this commit
except RuntimeError as e: | ||
raise RuntimeError(f"Failed to fetch component template metadata from cluster endpoint: {e!s}") |
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.
For the reasons mentioned on the previous comment, I'd rather see the RuntimeException be propagated rather than handling and obscuring the underlying cause.
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.
Exception chaining added with this commit
87f156d
to
d3c0435
Compare
Signed-off-by: Kartik Ganesh <gkart@amazon.com>
…template migration This includes class representations of component and index template information (and their unit tests). Index_operations now also has new functions to fetch and create component/index templates - unit test coverage for this is TBD. Finally, the template migration logic has been added to metadata_migration Signed-off-by: Kartik Ganesh <gkart@amazon.com>
d3c0435
to
917f273
Compare
@gregschohn This is done by including an extra flag with the orchestrator:
This is also captured in the code's argparse / help documentation |
Signed-off-by: Kartik Ganesh <gkart@amazon.com>
Signed-off-by: Kartik Ganesh <gkart@amazon.com>
Description
This change enables index template and component template migration from source to target cluster as a part of the metadata migration step of Fetch Migration. Now that metadata migration performs multiple distinct steps, these have been refactored into encapsulated functions rather than being inline in the
run
entrypoint.New functions have been added to
index_operations.py
to fetch and create component templates and index templates. TheIndexTemplateInfo
class derives from theComponentTemplateInfo
class since the former can include a "template" definition that overrides its components, and it includes additional fields (such as "composed_of", "priority" and "index_patterns")Unit tests have been added to maintain high test coverage.
Testing
Tested emperically by running Fetch Migration from a source cluster that had index templates and component templates defined, to a target cluster. The templates were correctly copied over (verified by diff-ing their structures)
Unit test coverage:
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.