Alternative approach: Let the model sort out the logic

[TinaHeiligers]

Summary

This explores an alternative approach to handling saved objects that can't be transformed.
The approach assumes we have PIT search and won't attempt to transform documents that have already failed transformation.
This approach does the following:

• migrate_run_docs uses a new method, migrateRawDocsNonThrowing that returns both the processed docs and the ids of those that couldn't be processed as arrays.
  • The new failedDocsIds contains the ids of those documents that we previously threw a CorruptSavedObjectError for.
  • The whole process flow for running migrations scripts is now wrapped in a try-catch, to catch any serious errors such as a migration script throwing an error.
• The migrationV2 model now does the following after OUTDATED_DOCUMENTS_SEARCH handls control flow to OUTDATED_DOCUMENTS_TRANSFORM:
  • If we have outdated documents passed in, we try to transform them
  • The response contains two arrays: { processedDocs: SavedObjectRawDoc[], failedDocs: string[] }
  • We now handle four cases based on what the result is:
    • If we don't have any failed docs and we haven't seen any failed docs yet (there aren't any on state), then we hand control flow off to TRANSFORMED_DOCUMENTS_BULK_INDEX.
      • TRANSFORMED_DOCUMENTS_BULK_INDEX bulk indexes the transformed documents and passes control flow back to OUTDATED_DOCUMENTS_SEARCH to carry on searching.
    • If we don't have any new failed docs but we've already encountered failed docs (collected on state previously), then we carry on searching.
    • If we have new failed docs from the transform but still haven't seen any other yet, then we add these failed document ids to a new state variable: failedDocumentIds: string[] and pass the control flow back to OUTDATED_DOCUMENTS_SEARCH. Note: We explicitly don't carry on indexing the transformed documents because we already know the migration will ultimately fail.
    • If we have new failed docs and we've already encountered failed docs (collected on state previously), then we add the newly failed docs to those on state (avoiding mutating state), and carry on searching.
  • Eventually, OUTDATED_DOCUMENTS_SEARCH will throw a CorruptSavedObject Error (that triggers dumping the logs) when it sees there aren't any more outdated documents to search and we have failed documents on state. If there aren't any failed documents on state and we've searched through all the outdated docs, we continue along the happy migration path.

All types were updated (I think) but no tests have been refactored yet. There's also an issue now that the model complains to not have a final return statement and that undefined is not declared on State.

PROBLEM: I'm not sure where the issue with the model not having a final return statement is coming from. Is it because we're not dealing with a Task?

[rudolf]

nit: I've been trying to avoid using exceptions for control flow. So basically we only throw an exception if something unexpected happens that we have no idea how to handle. If there's a known failure condition we go to the FATAL state. E.g. when there's a newer Kibana we want to fail, but this is a condition we fully expect to happen so we handle it explicitly https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrationsv2/model.ts#L715-L716

[TinaHeiligers]

Ok, that makes sense, especially because we're handling the failed docs as a right. Should we then rather handle this as a failure condition? We want migrations to stop after we've built up the full list of failed docs. Should we rather try the approach of creating a TaskEither from the async transform method?

[rudolf]

Instead of throwing we would just go to the FATAL state with a reason string. The code changed after I posted, so the link is no longer showing the correct lines.

But I meant something like:

        return {
          ...stateP,
          controlState: 'FATAL',
          reason: '... migrations failed because of the following corrupt saved object documents: [...]',
        };

[TinaHeiligers]

I thought about that too but then realized we're specifically targeting CorruptSavedObjectError within the catch block of migrationStateActionMachine, where we throw a new Error with a custom message. I guess we won't need to target CorruptSavedObjectError anymore if we're going to the FATAL state with a detailed reason.

[rudolf]

might be simpler to not make this property optional otherwise we have three states:

1. undefined
2. empty array (no failures)
3. non-empty array (some failures)

But we don't need or use (1), so we can rather initialize an empty array and then we don't need to use failedDocumentIds! in other places.

[TinaHeiligers]

abandoned approach in favor of elastic#96986