[Task Manager] Adds runNow api to Task Manager#51601
Merged
gmmorris merged 46 commits intoelastic:masterfrom Dec 16, 2019
Merged
[Task Manager] Adds runNow api to Task Manager#51601gmmorris merged 46 commits intoelastic:masterfrom
runNow api to Task Manager#51601gmmorris merged 46 commits intoelastic:masterfrom
Conversation
Contributor
💚 Build Succeeded |
Contributor
💚 Build Succeeded |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💔 Build Failed |
Contributor
|
Pinging @elastic/kibana-stack-services (Team:Stack Services) |
runNow api to Task ManagerrunNow api to Task Manager
Contributor
💚 Build Succeeded |
Contributor
💚 Build Succeeded |
pmuellr
reviewed
Dec 10, 2019
Contributor
pmuellr
left a comment
There was a problem hiding this comment.
Reviewed the code, not the tests (yet, anyway). Left a bunch of comments. I'd need more time to read/review this code to approve it - it's some pretty dense code, and I'm still n00bish on functional and observable.
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| export interface TermBoolClause { |
Contributor
There was a problem hiding this comment.
This pattern seems nice to me, I guess no one else has done something similar in Kibana already? OTOH, I'm a n00b at query DSL, so what do I know.
Contributor
Author
There was a problem hiding this comment.
I looked around but couldn't see anything like it, so went ahead and did it to clean it up here in this PR. :)
runNow api to Task ManagerrunNow api to Task Manager
7 tasks
Contributor
Author
|
@elasticmachine merge upstream |
mikecote
approved these changes
Dec 11, 2019
Contributor
mikecote
left a comment
There was a problem hiding this comment.
Took a look and code LGTM. Though I'm not expert enough to review the observable related code. I would defer to someone with more expertise to do a targeted review on those pieces.
Contributor
Author
|
@elasticmachine merge upstream |
…o task-manager/run-now
pgayvallet
approved these changes
Dec 13, 2019
Contributor
pgayvallet
left a comment
There was a problem hiding this comment.
only reviewed the task_poller.ts code.
As discussed on slack: There could be another approach that could remove the whole scan operator by tapping into the set from pollRequests$ before the merge, However both approaches relies on side-effects, so it's only subjectif on which is the best one.
LGTM to me
Contributor
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Contributor
Author
|
@elasticmachine merge upstream |
Contributor
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Dec 16, 2019
Adds a `runNow` api to Task Manager, allowing us to force the refresh of a recurring task. This PR includes a couple of sustainability changes as well as the feature itself. 1. **Declarative query composition.** At the moment the queries in the TaskStore are huge JSON objects that are hard to maintain and understand. This PR introduces a pattern where the different parts of the query are composed out of type-checked functions, making it easier to maintain and to construct dynamically as needs change. _This was included in this PR as the **markAvailableTasksAsClaimed** query needs different query clauses depending on whether there are specific Tasks we wish to claim first. 2. **Refactoring of the Task Poller** As the `runNow` api is introduced we find Task Manager's lifecycle in a weird state where it has both a _pull_ model, where timeouts & callbacks interact without having to responsd to any external requests, and a _push_ model where requests are made to the new `runNow` api. Balancing these two proved error prone, hard to maintain and had the potential of _lossy_ behaviour where requests are dropped accidentally. To address this TaskPoller has been refactored using Rxjs observables, remodelling the existing _pull_ mechanism as a _push_ mechanism so Task Manager can _respond_ to both _polling_ calls and _runNow_ in a similar fashion. And ofcourse the main feature of this PR: 3. **runNow api** An api on TaskManager that takes a _task ID_ and attempts to run the task. The call returns a promise which resolves with a result which notifies the caller when the task has either completed successfully, or result in an error.
gmmorris
added a commit
that referenced
this pull request
Dec 16, 2019
Adds a `runNow` api to Task Manager, allowing us to force the refresh of a recurring task. This PR includes a couple of sustainability changes as well as the feature itself. 1. **Declarative query composition.** At the moment the queries in the TaskStore are huge JSON objects that are hard to maintain and understand. This PR introduces a pattern where the different parts of the query are composed out of type-checked functions, making it easier to maintain and to construct dynamically as needs change. _This was included in this PR as the **markAvailableTasksAsClaimed** query needs different query clauses depending on whether there are specific Tasks we wish to claim first. 2. **Refactoring of the Task Poller** As the `runNow` api is introduced we find Task Manager's lifecycle in a weird state where it has both a _pull_ model, where timeouts & callbacks interact without having to responsd to any external requests, and a _push_ model where requests are made to the new `runNow` api. Balancing these two proved error prone, hard to maintain and had the potential of _lossy_ behaviour where requests are dropped accidentally. To address this TaskPoller has been refactored using Rxjs observables, remodelling the existing _pull_ mechanism as a _push_ mechanism so Task Manager can _respond_ to both _polling_ calls and _runNow_ in a similar fashion. And ofcourse the main feature of this PR: 3. **runNow api** An api on TaskManager that takes a _task ID_ and attempts to run the task. The call returns a promise which resolves with a result which notifies the caller when the task has either completed successfully, or result in an error.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
runNowapi to Task Manager, allowing us to force the refresh of a recurring task.This PR includes a couple of sustainability changes as well as the feature itself.
Declarative query composition. At the moment the queries in the TaskStore are huge JSON objects that are hard to maintain and understand. This PR introduces a pattern where the different parts of the query are composed out of type-checked functions, making it easier to maintain and to construct dynamically as needs change. _This was included in this PR as the markAvailableTasksAsClaimed query needs different query clauses depending on whether there are specific Tasks we wish to claim first.
Refactoring of the Task Poller As the
runNowapi is introduced we find Task Manager's lifecycle in a weird state where it has both a pull model, where timeouts & callbacks interact without having to responsd to any external requests, and a push model where requests are made to the newrunNowapi. Balancing these two proved error prone, hard to maintain and had the potential of lossy behaviour where requests are dropped accidentally. To address this TaskPoller has been refactored using Rxjs observables, remodelling the existing pull mechanism as a push mechanism so Task Manager can respond to both polling calls and runNow in a similar fashion.And ofcourse the main feature of this PR:
Open Issues:
There are still a couple of things to address in this PR:
There is now an unbounded buffer - we probably want to cap it at some configurable capacity.If the task being run by runNow fails, it is treated the same as any task that fails - it is rescheduled to try again, assuming there are more attempts available to it. This means a call to runNow might report a failure, and it might then succeed minutes later. Some thought needs to be put into how to address/communicate this.We've agreed this is a natural behaviour and we're fine with it,.while TaskPoller now uses a push model, it still relies on some pull models (fillPool mainly, with it's while(true), doesn't fit into the model well) that should also be refactored - this should result in Task Manager being clearner and easier to maintain in the future.several Perf metrics have been omitted by the changes to TaskPoller, we should figure out how to bring these back. And a fresh performance test needs to be run to ensure these changes haven't caused a regression.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportThis was checked for keyboard-only and screenreader accessibilityFor maintainers