-
Notifications
You must be signed in to change notification settings - Fork 634
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
Add checks for reindex task failures and number of docs created #1291
base: master
Are you sure you want to change the base?
Conversation
Sorry for the delays. I will hopefully get around to reviewing this further tonight. |
I will look this all over in a few days. I'm super busy with work right now. |
total_docs = response['task']['status']['total'] | ||
created_docs = response['task']['status']['created'] | ||
|
||
if not created_docs == total_docs: |
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.
So, devil's advocate here. What happens if someone does a reindex from query instead of a full index? Will these still be equal? What if someone reindexes from multiple indices to 1 index (e.g. 1 week's worth of daily indices into a single weekly index)? Do these numbers match up? If the answer is not "yes," then I can't use the code—at least not as-is.
The draft code works just fine in the event of a 1:1 reindex. I could merge it if the test is only made made when a verify_doc_count
flag is present (or something like it). It would, of course, have to be vetted by conditional tests. It wouldn't be valid, for example, if Curator was able to detect a many-to-one reindex (code which would have to be added).
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.
If you reindex going from a mapping file without nested documents to a mapping file with nested documents, the document count will be different. I assume the doc count verification should be optional...
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 providing a concrete example, @rahst12
Cross-reference #1299 |
Sorry for the hideous delay on this. I'm barely catching up. Can you rebase this so I can merge it? |
Currently curator simply checks if the destination index was created after a reindex task.
This is not enough for running this in production for us, as we want to delete the source indices after a successful reindex.
Therefore it can be the case, that the reindex task finishes with failures and curator does not throw an exception. The following actions will still get executed and in our case this means the source indices get deleted, even though the reindex task completed with a lot of errors.
Proposed Changes
Would be awesome if you could take a look at this. The implementation here is just a first draft.