-
Notifications
You must be signed in to change notification settings - Fork 62
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] Improve autodetect logic for persistence #437
[ML] Improve autodetect logic for persistence #437
Conversation
Changed the logic surrounding persistence of both state and quantiles on graceful shutdown so that persistence only occurs if and only if at least one input record has been processed or time has been advanced. closes elastic#393
if (m_NumRecordsHandled == 0) { | ||
LOG_DEBUG(<< "Zero records were handled - will not attempt to persist " | ||
<< description << "."); | ||
return false; |
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.
In the case of a categorizer chained to an anomaly detector this might not be correct - it's possible that time has been advanced on the anomaly detector.
So this method needs a // Pass on the request in case we're chained
step like some of the other methods in this class have. If the chained processor's isPersistenceNeeded()
returns true
then this method should return true
, otherwise it should make its own decision using the code that's there now.
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
I saw one nit, but I'm happy to merge as-is if you want to move onto something else.
lib/api/CFieldDataTyper.cc
Outdated
@@ -336,6 +336,11 @@ bool CFieldDataTyper::persistState(core::CDataAdder& persister) { | |||
} | |||
|
|||
bool CFieldDataTyper::isPersistenceNeeded(const std::string& description) const { | |||
// Pass on the request in case we're chained | |||
if (m_OutputHandler.isPersistenceNeeded(description) == 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.
nit: although we prefer == false
to !
, I think == true
is unnecessary for methods that clearly return a bool
Changed the logic surrounding persistence of both state and quantiles on graceful shutdown so that persistence only occurs if and only if at least one input record has been processed or time has been advanced. closes elastic#393
Changed the logic surrounding persistence of both state and quantiles on graceful shutdown so that persistence only occurs if and only if at least one input record has been processed or time has been advanced. closes elastic#393
Changed the logic surrounding persistence of both state and quantiles on graceful shutdown so that persistence only occurs if and only if at least one input record has been processed or time has been advanced. closes elastic#393
Similar to the fix for state and quantiles in elastic#437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Fixes elastic#394
Similar to the fix for state and quantiles in #437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Fixes #394
Similar to the fix for state and quantiles in elastic#437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of elastic#512
Similar to the fix for state and quantiles in elastic#437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of elastic#512
Similar to the fix for state and quantiles in elastic#437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of elastic#512
Similar to the fix for state and quantiles in #437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of #512
Similar to the fix for state and quantiles in #437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of #512
Similar to the fix for state and quantiles in #437, if no input is received and time is not advanced then there is no need to write model size stats when the autodetect process exits. Doing this can actually cause a problem for a job that has never ever seen any input, as the unnecessary model size stats were written with a negative timestamp. This change also adds an extra defensive check to prevent that ever happening, although the only situation when it is thought to be possible should be prevented by the first change. Backport of #512
Changed the logic surrounding persistence of both state and quantiles on
graceful shutdown so that persistence only occurs if and only if at
least one input record has been processed or time has been advanced.
closes #393