Create an annotation when a model snapshot is stored #53783
Conversation
przemekwitek
force-pushed
the
annotation_on_model_snapshot
branch
4 times, most recently
from
ed54e46 to
ef9fa5d
Compare
przemekwitek
changed the title
|
Pinging @elastic/ml-core (:ml) |
| @@ -66,7 +66,7 @@ public void clusterChanged(ClusterChangedEvent event) { | |||
| // The atomic flag prevents multiple simultaneous attempts to create the | |||
| // index if there is a flurry of cluster state updates in quick succession | |||
| if (event.localNodeMaster() && isIndexCreationInProgress.compareAndSet(false, true)) { | |||
| AnnotationIndex.createAnnotationsIndexIfNecessary(settings, client, event.state(), ActionListener.wrap( | |||
| AnnotationIndex.createAnnotationsIndexIfNecessary(client, event.state(), ActionListener.wrap( | |||
There was a problem hiding this comment.
This still leaves open the following bug (unsure how to fix it really).
- Node running the job is updated
- Job gets reallocated to a new node
- Job writes annotation for a model snapshot
- The master node is still Old, consequently never crated the annotations index
- Annotation persistence auto-created the index and now the UI fails to read from the index.
It seems to me that we need to verify if the index exists already and create it if necessary when we write annotations.
There was a problem hiding this comment.
I think that scenario was a problem in 6.7 and 6.8 as there were 6.x indices that didn't have the annotations index - it probably explains a few bug reports we had when the annotations index was not available as expected. But now any rolling upgrades must be starting from 6.8 at the earliest, so the old version will have the annotations index already.
There was a problem hiding this comment.
OK, my concern is that this will occur again whenever we update the mapping of the annotation index.
We can sort of 'boot' that for now I suppose. But we should definitely make sure that whomever updates the mapping of the annotations index knows that they need make this sort of change.
There was a problem hiding this comment.
OK, my concern is that this will occur again whenever we update the mapping of the annotation index.
My PR does not update mappings of annotations index.
The new annotations I'm adding fit the existing mappings so there was no point in changing them.
przemekwitek
force-pushed
the
annotation_on_model_snapshot
branch
6 times, most recently
from
ea8be29 to
c9615ca
Compare
There was a problem hiding this comment.
Looks good. I just left a couple of minor comments.
After this is merged I think this would also be good to test on a real cluster that's left running for a few days with a real-time job so that the effect of periodic model snapshots and nightly maintenance can be observed, and the annotations viewed in Kibana.
...java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java
Outdated
Show resolved
Hide resolved
...java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -66,7 +66,7 @@ public void clusterChanged(ClusterChangedEvent event) { | |||
| // The atomic flag prevents multiple simultaneous attempts to create the | |||
| // index if there is a flurry of cluster state updates in quick succession | |||
| if (event.localNodeMaster() && isIndexCreationInProgress.compareAndSet(false, true)) { | |||
| AnnotationIndex.createAnnotationsIndexIfNecessary(settings, client, event.state(), ActionListener.wrap( | |||
| AnnotationIndex.createAnnotationsIndexIfNecessary(client, event.state(), ActionListener.wrap( | |||
There was a problem hiding this comment.
I think that scenario was a problem in 6.7 and 6.8 as there were 6.x indices that didn't have the annotations index - it probably explains a few bug reports we had when the annotations index was not available as expected. But now any rolling upgrades must be starting from 6.8 at the earliest, so the old version will have the annotations index already.
przemekwitek
force-pushed
the
annotation_on_model_snapshot
branch
from
c9615ca to
8717e21
Compare
|
run elasticsearch-ci/default-distro |
przemekwitek
force-pushed
the
annotation_on_model_snapshot
branch
from
03b0c58 to
70fbc6e
Compare
| modelSnapshot.getTimestamp(), | ||
| modelSnapshot.getTimestamp(), |
There was a problem hiding this comment.
I wonder if these should have been modelSnapshot.getLatestResultTimeStamp()?
See #56076 (comment) for more details on this.
There was a problem hiding this comment.
I've just raised #56093 to address that.
This PR creates an annotation every time a model snapshot is stored and deletes an annotation when the model snapshot is deleted.
It is achieved the following way:
AnnotationPersisterclass in a separate file.AnnotationPersisteris used inDatafeedJobwhich creates annotations for delayed dataAnnotationPersisteris re-used inAutodetectResultProcessoron every newModelSnapshotobject received from C++JobDataDeleteris extended so that it also deletes model snapshot annotation from.ml-annotations*indexPoints 1. and 2. is just refactoring.
Point 3. is the new behavior.
Relates #52149