[ML] Adjust finalize job action to work with documents

[davidkyle]

For the JIndex feature branch.

The Finalize job action sets the job finished time and should be called only when the job is closed. The change is fairly simple: add an internal finished time field to JobUpdate and update the job in the action.

I take it as a sign of progress that this change can cause Version conflict exceptions between AutoDetectResultProcessor and the finalize action as both attempt to update the job. I marked this WIP as I attempt to resolve the problem some options are:

1. Reduce contention. AutoDetectResultProcessor is trying to set established model memory could this be stored somewhere else?
2. Retry on failure.
3. 1 and 2

[elasticmachine]

Pinging @elastic/ml-core

[droberts195]

++ to

3. 1 and 2

Due to #34084 established_model_memory can be removed from the Job object. (Parsing must tolerate it for BWC, but it doesn't need to be stored or written.)

[davidkyle]

The version conflict was due to setting the model snapshot id. This causes a job update which in turn makes an async call to JobManager.validate between reading the document and writing it back. The validate method can take some time hence the conflict as the document has changed during that call. JobManager.validate checks the model snapshot document exists, but we know AutodetectResultsProcessor has just indexed the doc so it is safe just to update the field in the config.

To prevent contention between setting the finished time when closing the job and the updates AutodetectResultsProcessor makes I moved setting the finished time to when the results processor finishes which is when the autodetect process stops (or fails). This makes more sense in the case of a crash as the finished time will be the time autodetect stopped rather than when the job was force closed. After this change there was no need for the FinalizeJobAction so I removed it but this may be a bad idea in a mixed cluster as a closing job on an old node will try to call that action and it no longer exists. I'm not sure what should be done here, maybe retain FinalizeJobAction and make it a no-op.

The updates made by autodetect are not ES updates without validation rather than using the job update action, this makes sense for internal updates where a single field is changing and also allows the use of retryOnConflict

The AutodetectResultsProcessor code can be simplified and there is still a race between setting the model snapshot Id and the finished time/established model memory but I'd prefer to wait until #34084 has been resolved as that will touch the same code.

[droberts195]

After this change there was no need for the FinalizeJobAction so I removed it but this may be a bad idea in a mixed cluster as a closing job on an old node will try to call that action and it no longer exists. I'm not sure what should be done here, maybe retain FinalizeJobAction and make it a no-op.

I think we need to know what the effect of this is in a mixed node cluster. If the older version node still closes the job it may be OK as it is. If the older node suffers an exception that causes the job to go into the failed state or returns a scary exception to the end user then we'll need to keep the FinalizeJobAction around as a no-op until 7.last.

[davidkyle]

I pushed more commits making the finalize job action a no-op. The action will be a no-op that isn't called in 6.last (6.5 can call it) and so can be removed in 7.

[droberts195]

I left a couple of questions about BWC wire formats and inconsistencies in where isInternal has been removed.

[droberts195]

Do we need to still read from the wire using something like this?

    // TODO change CURRENT to specific version when feature branch is merged
    if (in.getVersion().onOrAfter(Version.V_6_3_0) && in.getVersion().before(Version.CURRENT)) {
        in.readBoolean(); // was waitForAck
    }

[davidkyle]

I don't know what I was thinking I should have reviewed this before asking for a review sorry. I pushed 2 more commits one addresses this and the other adds the finalize job action back into the security tests.

[droberts195]

Similar to read, if writing to a version in between 6.3.0 and the version the feature branch gets merged into should this write true so the wire format is as expected on the remote end?

[droberts195]

The isInternal data member hasn't been removed - is it correct to remove it here? (If it is for some reason then it should also be removed from hashCode().)

[droberts195]

LGTM