Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] Add reason to DataFrameAnalyticsTask updateState log message #52659

Merged
merged 3 commits into from
Feb 24, 2020
Merged

[ML] Add reason to DataFrameAnalyticsTask updateState log message #52659

merged 3 commits into from
Feb 24, 2020

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Feb 21, 2020 •
edited
Loading

When setting the state to FAILED the reason isn't logged. Debugging would be simpler if it was.

It looks to me like DataFrameAnalyticsTask.updateState() is only ever called with DataFrameAnalyticsState.FAILED the non failure updates call AllocatedPersistentTask.updatePersistentTaskState(). @przemekwitek what do you think of renaming this method to setFailed as this method has the extra logging we are not really interesting in when setting the state to STARTED etc.

For #52654

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

benwtrent
benwtrent approved these changes Feb 21, 2020
View reviewed changes
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% we should include the reason :)

@przemekwitek
Copy link
Contributor

@przemekwitek what do you think of renaming this method to setFailed

SGTM if that's what the method is used for.

przemekwitek
przemekwitek approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidkyle
Copy link
Member Author

davidkyle commented Feb 24, 2020 •
edited
Loading

@przemekwitek I made the refactor of renaming the method to setFailed and removed the nullable annotation from the reason parameter. Can you take another look please

@przemekwitek
Copy link
Contributor

przemekwitek commented Feb 24, 2020 •
edited
Loading

@przemekwitek I made the refactor of renaming the method to setFailed and removed the nullable annotation from the reason parameter. Can you take another look please

LGTM2
Thanks for making this part more readable.

@davidkyle davidkyle merged commit 28582d8 into elastic:master Feb 24, 2020
@davidkyle davidkyle deleted the add-reason branch February 24, 2020 11:57
@davidkyle davidkyle removed the v7.6.1 label Feb 24, 2020
@davidkyle davidkyle mentioned this pull request Feb 24, 2020
Merged
davidkyle added a commit to davidkyle/elasticsearch that referenced this pull request Feb 24, 2020
@davidkyle
[ML] Add reason to DataFrameAnalyticsTask setFailed log message (elas…
bfe87c8 
…tic#52659)
davidkyle added a commit that referenced this pull request Feb 24, 2020
@davidkyle
[ML] Add reason to DataFrameAnalyticsTask setFailed log message (#52659…
044a4e1 
…) (#52707)
@davidkyle davidkyle mentioned this pull request Feb 24, 2020
Closed
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benwtrent benwtrent benwtrent approved these changes

@przemekwitek przemekwitek przemekwitek approved these changes

Assignees
No one assigned
Labels
:ml Machine learning v7.7.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidkyle @elasticmachine @przemekwitek @benwtrent @jakelandis