-
Notifications
You must be signed in to change notification settings - Fork 25k
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
[ML] Introduce a "starting" datafeed state for lazy jobs #53918
Changes from 2 commits
c386dc2
a08fda3
23e48cf
e31bea9
85e5fbc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -537,7 +537,7 @@ void closeJob(String reason) { | |||
* Important: the methods of this class must NOT throw exceptions. If they did then the callers | ||||
* of endpoints waiting for a condition tested by this predicate would never get a response. | ||||
*/ | ||||
private class JobPredicate implements Predicate<PersistentTasksCustomMetaData.PersistentTask<?>> { | ||||
private static class JobPredicate implements Predicate<PersistentTasksCustomMetaData.PersistentTask<?>> { | ||||
|
||||
private volatile boolean opened; | ||||
private volatile Exception exception; | ||||
|
@@ -554,6 +554,7 @@ public boolean test(PersistentTasksCustomMetaData.PersistentTask<?> persistentTa | |||
|
||||
// This means we are awaiting a new node to be spun up, ok to return back to the user to await node creation | ||||
if (assignment != null && assignment.equals(JobNodeSelector.AWAITING_LAZY_ASSIGNMENT)) { | ||||
opened = true; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I changed my mind on - look at the difference between the two commits on this PR to see. If we return You could say that the UI should be changed to accept
So, the response from the open job endpoint has to be sufficient for the UI to answer the question "is it appropriate to start the datafeed?" I think there are two options:
Interestingly start data frame analytics currently does return Line 404 in d2740c2
Which do you prefer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I suggest a 3rd option? We can stick to interpreting "opened" and "started" as "it is now opened|started". The UI could simply check the status code for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sure, that is another option. I think that would be a trivial change in the UI code because already >= 400 status codes go through a different control path so it would basically be removing a single I noticed that docs currently don't mention the possibility that The final thing to consider is that starting a data frame analytics job does currently return an acknowledged response with a value hardcoded to None of these options are particularly difficult to implement so I will wait until Monday morning once others have had a chance to comment before making any more code changes to avoid unnecessary churn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. Note that I don't feel strongly about whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parity with analytics and anomaly is useful, as is treatment for the UI. Is there a possibility to add a second response There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think the least BWC-breaking way we can move towards that is to always have
This is quite similar to the I prefer
then I would probably have to read the docs to work out how to interpret that. Also:
is what most users would see, and that looks like it contains redundancy. Whereas:
and:
seem easier to interpret without reading the docs. So unless anyone strongly objects I will eventually implement that. But I'd rather get this PR merged ASAP since it fixes a nasty bug and then add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, let's merge PR to fix the bug. And let's treat the second field as a separate discussion for a future release. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I opened #54067 to discuss what to do in 7.8. |
||||
return true; | ||||
} | ||||
|
||||
|
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.
The
stopping
state is not introduced by this PR - it has existed for a couple of years. But I thought I'd add it while I was modifying the list.(This PR exposes the
starting
state externally for the first time, so it's understandable it wasn't previously documented.)