[ML] Add control message to start background persistence #19

davidkyle · 2018-03-16T18:06:05Z

Starts background persistence if not currently persisting and resets the clock on the periodic persist.

This is tested through an integration test in the x-pack repo

davidkyle · 2018-03-16T18:06:46Z

lib/api/CAnomalyJob.cc

@@ -393,6 +393,14 @@ bool CAnomalyJob::handleControlMessage(const std::string &controlMessage)
        case 'p':
            this->doForecast(controlMessage);
            break;
+        case 'w':


'p' for persistence and 's' for save are already taken so 'w' for write.

edsavage

LGTM

droberts195 · 2018-03-19T09:15:41Z

lib/api/CAnomalyJob.cc

@@ -393,6 +393,14 @@ bool CAnomalyJob::handleControlMessage(const std::string &controlMessage)
        case 'p':
            this->doForecast(controlMessage);
            break;
+        case 'w':
+            {


In the future we'll probably also want the option to request persistence in the foreground.

This could be indicated using a second letter after the 'w', say 'b' for background and 'f' for foreground.

Obviously all this could be introduced in the future PR where the foreground persistence option is added. However, it would make adding that change smoother (with CI interaction between repos) if the second letter was added now, with a warning message if no second letter was present or was not understood.

👍 I like the idea of persist foreground and background.

I wanted to propose a different change I was going to do in another PR but I'll raise it now. This command is added for when the datafeed has finished lookback, it's possible that if a lookback took more than 3 hours (or a multiple of) then it would have recently persisted. How about an optional duration following the 'b' which would be interpreted as 'start bg persist now if you have not done it in the last N minutes'

Sounds good. That also answers the question about what to do if a background persist is currently in progress when the message is received - it would be reasonable not to do it ever under these circumstances.

'start bg persist now if you have not done it in the last N minutes'

I would change 'minutes' to 'seconds' in the command, as everything else works in terms of seconds in the C++.

droberts195 · 2018-03-19T09:41:23Z

lib/api/CBackgroundPersister.cc

@@ -178,6 +178,11 @@ bool CBackgroundPersister::firstProcessorPeriodicPersistFunc(const TFirstProcess
    return true;
 }

+bool CBackgroundPersister::startBackgroundPersist(void)
+{
+    return this->startBackgroundPersist(core::CTimeUtils::now());


If this is called while a background persist is currently in progress then there are two potential problems:

It won't actually do another background persist, but the only indication of this will be the error message "Failed to start background persistence" - there will be no extra explanation of the reason why.

Before it fails it will copy all the models. We know that background persist can lead to a doubling of process memory usage, but this situation would lead to a temporary tripling of process memory.

One way to solve these problems is as follows:

Add comments to CBackgroundPersister::startPersist() and CBackgroundPersister::startBackgroundPersist(core_t::TTime) saying that they must only be called when m_Mutex is locked. I think this is OK for private methods.

Change m_Mutex from a core::CFastMutex to a core::CMutex, as it's going to be held for longer due to the rest of this proposal. (Or change it to a std::mutex if you prefer.)

Before the if (this->isBusy()) check in CBackgroundPersister::startBackgroundPersistIfAppropriate(), lock m_Mutex.

At the beginning of CBackgroundPersister::startBackgroundPersist() (i.e. the public method), lock m_Mutex. Then, in between the mutex acquisition and return this->startBackgroundPersist(core::CTimeUtils::now()); check if (this->isBusy()) and log a warning message that clearly states why the requested background persist is not being run.

With (4) there is also the question of whether it's acceptable to ignore a requested background persist if there's already one in progress. If we think it's not acceptable then we need functionality to remember the request and have the background thread run again as soon as it finishes. But it's probably best to discuss that question as a separate issue and defer any changes to a future PR rather than mix it into this change.

another alternative: using the CConcurrentWrapper, that would allow to queue persistence requests.

Good point. If m_FirstProcessorPeriodicPersistFunc is called while isBusy() == true then it will result in concurrent modification of the m_PersistFuncs member.

I think it is sufficient to have the isBusy() check at the start of the public startBackgroundPersist(void) function and log a message. I think it's acceptable to ignore the persist request if one is currently executing. I've pushed a change with that check

another alternative: using the CConcurrentWrapper, that would allow to queue persistence requests

But I think we need to consider carefully if it's sensible to queue persistence requests. If state is persisted twice in quick succession all that really achieves is to waste CPU.

true, but flooding protection is on a different page just saying it allows to, does not mean we should do it. Same is true for the other way around: A scheduled background persist could kick in right after a on-demand persist. Should not be to hard to check whether persistence makes sense or not.

A scheduled background persist could kick in right after a on-demand persist

A requested persist resets the periodic persist clock so it won't happen again for another 3+ hours

I think it is sufficient to have the isBusy() check at the start of the public startBackgroundPersist(void) function and log a message.

That's only true because the check for a periodic background persist is triggered in the same thread as control messages are handled in, so they cannot both happen at the same time.

If you're not going to extend the mutex locking then I think the Doxygen comments need to say that only one thread in the program is allowed to call the startBackgroundPersist() and startBackgroundPersistIfAppropriate() methods.

I've documented the thread safety restrictions

droberts195

LGTM

As MVP for 6.3 to support Cloud I think this is enough. Further changes can be done later.

* Add control message for back ground persist * Add isBusy check to public startBackgroundPersist function * Document thread safety aspects

Add control message for back ground persist

4096258

davidkyle added v7.0.0 v6.3.0 review labels Mar 16, 2018

davidkyle commented Mar 16, 2018

View reviewed changes

edsavage approved these changes Mar 19, 2018

View reviewed changes

droberts195 reviewed Mar 19, 2018

View reviewed changes

davidkyle removed the v6.3.0 label Mar 19, 2018

Add isBusy check to public startBackgroundPersist function

79e2234

droberts195 added the :ml label Mar 19, 2018

davidkyle added the v6.3.0 label Mar 19, 2018

Document thread safety aspects

67e0b85

droberts195 approved these changes Mar 19, 2018

View reviewed changes

davidkyle merged commit 5fde005 into elastic:master Mar 19, 2018

davidkyle deleted the persist-now branch March 19, 2018 17:17

davidkyle added a commit that referenced this pull request Mar 19, 2018

[ML] Add control message to start background persistence (#19)

3f06de9

* Add control message for back ground persist * Add isBusy check to public startBackgroundPersist function * Document thread safety aspects

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Add control message to start background persistence (#19)

eb300d4

* Add control message for back ground persist * Add isBusy check to public startBackgroundPersist function * Document thread safety aspects

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Add control message to start background persistence (#19)

34fef02

* Add control message for back ground persist * Add isBusy check to public startBackgroundPersist function * Document thread safety aspects

lcawl added the >bug label Jun 8, 2018

davidkyle mentioned this pull request Jun 20, 2023

[NLP] Catch exceptions thrown during inference and report as errors #2542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Add control message to start background persistence #19

[ML] Add control message to start background persistence #19

davidkyle commented Mar 16, 2018

davidkyle Mar 16, 2018 •

edited

Loading

edsavage left a comment

droberts195 Mar 19, 2018

davidkyle Mar 19, 2018 •

edited

Loading

droberts195 Mar 19, 2018

droberts195 Mar 19, 2018

hendrikmuhs Mar 19, 2018

davidkyle Mar 19, 2018

droberts195 Mar 19, 2018

hendrikmuhs Mar 19, 2018

davidkyle Mar 19, 2018 •

edited

Loading

droberts195 Mar 19, 2018

davidkyle Mar 19, 2018

droberts195 left a comment

[ML] Add control message to start background persistence #19

[ML] Add control message to start background persistence #19

Conversation

davidkyle commented Mar 16, 2018

davidkyle Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

edsavage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

davidkyle Mar 16, 2018 •

edited

Loading

davidkyle Mar 19, 2018 •

edited

Loading

davidkyle Mar 19, 2018 •

edited

Loading