-
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
Snapshot status #757
Snapshot status #757
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #757 +/- ##
=============================================
+ Coverage 68.50% 88.13% +19.63%
=============================================
Files 270 48 -222
Lines 11175 3059 -8116
Branches 736 0 -736
=============================================
- Hits 7655 2696 -4959
+ Misses 3118 363 -2755
+ Partials 402 0 -402
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bdff322
to
f268d4c
Compare
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.
Looks good! A couple questions, but no blockers.
@@ -72,40 +70,10 @@ public Path unpack() { | |||
} | |||
} | |||
|
|||
@Override | |||
public void close() { |
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.
do we care about these things not getting cleaned up?
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.
No, we want to recycle tasks between shards with ECS. When this was in there, we were deleting the shard date before we reindexed it
IN_PROGRESS = "IN_PROGRESS" | ||
|
||
|
||
def convert_snapshot_state_to_status(snapshot_state: str) -> Tuple[SnapshotStatus, str]: |
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'm not sure I understand why we should have State & Status and what the difference is. Should we just use these States throughout?
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.
State is the cluster response enum whereas Status is what we define. I had originally defined it when we were also storing status in a separate doc.
This helps us maintain a consistent Status despite state renaming across ES/OS versions
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.
This doesn't feel right, we are embedding version handling deep in the console. I understand getting this in position for the demo, but this blends the responsibilities of the system quite a bit - it is going to make our codebase hard to maintain very quickly.
@AndreKurait What do you think of this, how should we capture the responsibility questions?
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.
Moved this to just return the state string that the cluster responds with
# Add sourcing of the completion script to .bashrc for persistence across sessions | ||
echo '. /etc/bash_completion' >> ~/.bashrc | ||
|
||
echo "Bash completion for console command has been set up and enabled." |
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 adding this!
e7597ba
to
3351c06
Compare
Signed-off-by: Andre Kurait <akurait@amazon.com> Add Snapshot Status updates Signed-off-by: Andre Kurait <akurait@amazon.com> Snapshot improvements Signed-off-by: Andre Kurait <akurait@amazon.com> Use new Rfs Document Migration Main Signed-off-by: Andre Kurait <akurait@amazon.com> Fix create snapshot Signed-off-by: Andre Kurait <akurait@amazon.com> Updates Signed-off-by: Andre Kurait <akurait@amazon.com> Improvements Signed-off-by: Andre Kurait <akurait@amazon.com> Improve RFS log4j2.xml Signed-off-by: Andre Kurait <akurait@amazon.com> revert documentReindexer Signed-off-by: Andre Kurait <akurait@amazon.com> add logging for documentReindexer Signed-off-by: Andre Kurait <akurait@amazon.com> remove logging Signed-off-by: Andre Kurait <akurait@amazon.com> Update printing based on feedback Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
Signed-off-by: Andre Kurait <akurait@amazon.com>
af22643
to
2c3058b
Compare
Signed-off-by: Andre Kurait <akurait@amazon.com>
2c3058b
to
95aa33b
Compare
IN_PROGRESS = "IN_PROGRESS" | ||
|
||
|
||
def convert_snapshot_state_to_status(snapshot_state: str) -> Tuple[SnapshotStatus, str]: |
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.
This doesn't feel right, we are embedding version handling deep in the console. I understand getting this in position for the demo, but this blends the responsibilities of the system quite a bit - it is going to make our codebase hard to maintain very quickly.
@AndreKurait What do you think of this, how should we capture the responsibility questions?
...erSolution/src/main/docker/migrationConsole/lib/console_link/console_link/models/snapshot.py
Show resolved
Hide resolved
Signed-off-by: Andre Kurait <akurait@amazon.com>
ae734ee
to
57016fc
Compare
Signed-off-by: Andre Kurait <akurait@amazon.com>
57016fc
to
32b0d9f
Compare
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 the updates @AndreKurait
Description
Implements several changes for performance and usability:
--wait
optionIssues Resolved
MIGRATIONS-1792
Is this a backport? If so, please add backport PR # and/or commits #
Testing
Lots of manual testing locally and in the cloud
Follow up for more documentation and E2E cloud testing https://opensearch.atlassian.net/browse/MIGRATIONS-1808
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.