-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Reindex resume from status #49255
Reindex resume from status #49255
Conversation
Reindex now uses last known good status on failure to ensure that counts continue from where it left when a node fails. Also added `persistentTaskId` to `StartReindexResponse` and removed a couple of obsolete todos. Relates elastic#42612
Pinging @elastic/es-distributed (:Distributed/Reindex) |
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.
Mostly looks good, just some questions about naming that we can discuss tomorrow if necessary.
@@ -244,6 +248,25 @@ public void rethrottle(float newRequestsPerSecond) { | |||
} | |||
} | |||
|
|||
public void resetStatus(BulkByScrollTask.Status checkpointStatus) { |
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.
Maybe initStatus
? or setInitialStatus
?
out.writeOptionalWriteable(reindexResponse); | ||
} | ||
|
||
public String getTaskId() { | ||
return taskId; | ||
} | ||
|
||
public String getPersistentTaskId() { |
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 was just working on this on a different branch and I feel like the persistent task id should be the default naming? As in the methods are getTaskId
or getReindexTaskId
? And the other one be getEphemeralTaskId
? Or getCoordinatorNodeTaskId
? Or getInitialEphemeralTaskId
?
…_resume_from_status
…_resume_from_status
…_resume_from_status
…_resume_from_status
…_resume_from_status
…_resume_from_status
@elasticmachine run elasticsearch-ci/packaging-sample-matrix-unix |
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.
LGTM
Reindex now uses last known good status on failure to ensure that counts
continue from where it left when a node fails.
Also added
persistentTaskId
toStartReindexResponse
and removed acouple of obsolete todos.
Relates #42612