[Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks #50232

gmmorris · 2019-11-11T23:23:16Z

Summary

Adds an ensureScheduled api to Task Manager which allow safer rescheduling of existing tasks by handling the case where a Task with a known ID is scheduled and clashes with an existing schedule of that same task.

fixes #41827
fixed #32050

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…at it does

elasticmachine · 2019-11-11T23:23:57Z

Pinging @elastic/kibana-stack-services (Team:Stack Services)

elasticmachine · 2019-11-12T00:42:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1d7e751

…is included in lens, maps and telemetry

elasticmachine · 2019-11-12T17:23:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: d90a483

elasticmachine · 2019-11-12T18:33:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 0e40e28

gmmorris · 2019-11-12T21:17:59Z

@elasticmachine merge upstream

elasticmachine · 2019-11-12T22:41:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4704660

elasticmachine · 2019-11-12T23:10:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: adbc0bc

elasticmachine · 2019-11-13T16:03:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dc84500

elasticmachine · 2019-11-13T17:50:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cdf9ec5

mikecote · 2019-11-14T13:30:30Z

I don't see any issues calling it ensureScheduling unless others have a better suggestion (ensureScheduled?).

The interface ExistingTaskInstance is a bit strange as this API also takes non existing task instances but I get it needs to have an id. Another way could be TaskInstanceWithId.

gmmorris · 2019-11-14T14:07:28Z

The interface ExistingTaskInstance is a bit strange as this API also takes non existing task instances but I get it needs to have an id. Another way could be TaskInstanceWithId.

Yeah, I was really unsure with that naming :)
TaskInstanceWithId could work, still feels weird 😆 ... but I guess ExistingTaskInstance is actually wrong, so I agree it should change... I'll think about it a bit more.

bmcconaghy

Code LGTM. Thanks for tackling this.

pmuellr

LGTM, but made a comment about changing a comment :-)

pmuellr · 2019-11-14T15:11:45Z

x-pack/legacy/plugins/task_manager/task.ts

+   * The id of the Elastic document that stores this instance's data. This can
+   * be passed by the caller when scheduling the task.
+   */
+  id: string;


wow, TIL. You can subclass an interface, changing a super-interface's property from optional to not optional. Neat-O!

That's changed now, as I'm using a new Utility type marking id as Required ;)
But the name is now TaskInstanceWithId.

pmuellr · 2019-11-14T15:16:16Z

x-pack/legacy/plugins/task_manager/task_manager.ts

+  public async ensureScheduling(
+    taskInstance: ExistingTaskInstance,
+    options?: any
+  ): Promise<ExistingTaskInstance> {


So I guess sometimes this will return a ConcreteTaskInstance (if not already scheduled), but otherwise the taskInstance sent in. Seems like it would be "nice" if it did always return a ConcreteTaskInstance, but guessing you will have to do another read to get that, and hardly seems worth it, since the caller may not need it. Seems like this should be fine as a first attempt, maybe we'd need to re-look at a way to get the ConcreteTaskInstance back later, if we find we need it for some reason.

In any case, the JSDoc comment above this should be changed to say it's returning an ExistingTaskInstance instead of ConcreteTaskInstance.

Yeah, I did think about that, though, the type it returns will always be TaskInstanceWithId, never an explicit ConcreteTaskInstance, as TaskInstanceWithId is essentially a subtype of ConcreteTaskInstance, but you're right that otherwise we'd have to make an additional read and it didn't seem worth it.
Feels reasonable to leave it such due to the ensure nature, no?

I'll change the JSDoc, thanks for catching 👍

pmuellr · 2019-11-14T15:21:19Z

Same feels on the new names as everyone, I guess. ensureScheduled sounds better to me than ensureScheduling. TaskInstanceWithId sounds better than ExistingTaskInstance.

gmmorris · 2019-11-14T15:32:57Z

Same feels on the new names as everyone, I guess. ensureScheduled sounds better to me than ensureScheduling.

Weird, to me ensureScheduled feels grammatically wrong... while ensureScheduling feels right? 🤷‍♂ Might be a US<->UK thing... anyway, happy to go with the majority on this one. :)

TaskInstanceWithId sounds better than ExistingTaskInstance.

Already changed :)

pmuellr · 2019-11-14T15:36:16Z

x-pack/legacy/plugins/task_manager/task.ts

@@ -10,6 +10,8 @@ import Joi from 'joi';
 * Type definitions and validations for tasks.
 */

+type Require<T extends object, P extends keyof T = keyof T> = Omit<T, P> & Required<Pick<T, P>>;


I think this new Require type could use a short comment - not immediately clear what it's doing :-)

wylieconlon · 2019-11-14T16:01:43Z

x-pack/legacy/plugins/lens/server/usage/task.ts

@@ -82,7 +82,7 @@ function scheduleTasks(server: Server) {
    // function block.
    (async () => {
      try {
-        await taskManager.schedule({
+        await taskManager.ensureScheduled({


wylieconlon

Lens part LGTM

gmmorris · 2019-11-14T16:21:52Z

@elasticmachine merge upstream

elasticmachine · 2019-11-14T17:38:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9d77ebc

…of existing tasks (elastic#50232) Adds an ensureScheduling api to Task Manager which allow safer rescheduling of existing tasks by handling the case where a Task with a known ID is scheduled and clashes with an existing schedule of that same task.

…of existing tasks (#50232) (#50705) Adds an ensureSchedule api to Task Manager which allow safer rescheduling of existing tasks by handling the case where a Task with a known ID is scheduled and clashes with an existing schedule of that same task.

elasticmachine · 2019-11-14T20:32:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9d77ebc

…ger-ace-theme * 'master' of github.com:elastic/kibana: (54 commits) [ML] Fixes word wrap in Overview page sidebar on IE (elastic#50668) Upgrade to TypeScript 3.7.2 (elastic#47188) fix: hide 'edit' button for mobile for dashboards (elastic#50639) fixes conditional links tests (elastic#50642) [SIEM] Fix IE11 timeline drag and drop issue (elastic#50528) [SIEM] Add SavedQuery in Timeline (elastic#49813) chore(NA): remove code plugin from codeowners (elastic#50451) [DOCS] Adds documentation on telemetry settings (elastic#50739) [Logs UI] Add IE11-specific CSS fixes for anomalies table (elastic#49980) [DOCS][SIEM]: Change Kibana advanced settings to match UI (elastic#50679) Change URLs for support menu (elastic#50700) [Reporting] Remove any types and references to Hapi (elastic#49250) [DOCS] Adds note about backups to Upgrade doc (elastic#50525) [Logs UI] Improve infra plugin compatibility with TS 3.7 (elastic#50491) [Task manager] Adds ensureScheduling api to allow safer rescheduling of existing tasks (elastic#50232) [DOCS] Adds link to content security policy doc (elastic#50698) Remove duplicate but in error message (elastic#50530) [ML] DF Analytics: Ensure creation flyout can be opened when no jobs exist (elastic#50417) Add filebeat notice (elastic#49065) [Monitoring] De-duplicate pipeline ids based on the ephemeral_id changing (elastic#49978) ... # Conflicts: # x-pack/legacy/plugins/grokdebugger/public/components/grok_debugger/brace_imports.ts

…of existing tasks (elastic#50232) Adds an ensureScheduling api to Task Manager which allow safer rescheduling of existing tasks by handling the case where a Task with a known ID is scheduled and clashes with an existing schedule of that same task.

gmmorris added 2 commits November 11, 2019 18:14

feat(task-manager): added scheduleIfNotExists api

090e094

refactor(task-manager): renamed scheduleIfNotExists api to clarify wh…

1d7e751

…at it does

gmmorris requested a review from a team November 11, 2019 23:23

gmmorris added Feature:Task Manager Team:Stack Services v7.6.0 v8.0.0 labels Nov 11, 2019

gmmorris added the release_note:enhancement label Nov 11, 2019

gmmorris added 3 commits November 12, 2019 11:04

fix(task-manager): fixed typing

aacff4d

Merge branch 'master' into task-manager/scheduleIfNotExists

18542d9

feat(task-manager): use ensureScheduling whenever the ID of the task …

d90a483

…is included in lens, maps and telemetry

gmmorris requested a review from a team November 12, 2019 16:24

Merge branch 'master' into task-manager/scheduleIfNotExists

0e40e28

elasticmachine and others added 2 commits November 12, 2019 13:18

Merge branch 'master' into task-manager/scheduleIfNotExists

4704660

Merge branch 'master' into task-manager/scheduleIfNotExists

adbc0bc

Merge branch 'master' into task-manager/scheduleIfNotExists

dc84500

doc(task-manager): document ensureShceduling api

cdf9ec5

bmcconaghy approved these changes Nov 14, 2019

View reviewed changes

refactor(task-manager): introduced a more explicit type for Task with ID

a7d12c5

pmuellr approved these changes Nov 14, 2019

View reviewed changes

refactor(task-manager): renamed ensureScheduling to ensureScheduled

7198e4c

pmuellr reviewed Nov 14, 2019

View reviewed changes

gmmorris added 2 commits November 14, 2019 10:37

refactor(task-manager): removed unnnecesery typing

55499bc

doc(task-manager): Document Require utility type

97ff0aa

wylieconlon reviewed Nov 14, 2019

View reviewed changes

wylieconlon approved these changes Nov 14, 2019

View reviewed changes

Merge branch 'master' into task-manager/scheduleIfNotExists

9d77ebc

gmmorris merged commit d777128 into elastic:master Nov 14, 2019

gmmorris mentioned this pull request Nov 14, 2019

[7.x] [Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks (#50232) #50705

Merged

gmmorris changed the title ~~[Task manager] Adds ensureScheduling api to allow safer rescheduling of existing tasks~~ [Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks Nov 14, 2019

gmmorris added the backported label Nov 14, 2019

gmmorris deleted the task-manager/scheduleIfNotExists branch November 21, 2019 13:46

gmmorris mentioned this pull request Nov 27, 2019

[7.5] [Task manager] Adds ensureScheduling api to allow safer rescheduling of existing tasks (#50232) #51796

Closed

gmmorris mentioned this pull request Nov 27, 2019

[7.4] [Task manager] Adds ensureScheduling api to allow safer rescheduling of existing tasks (#50232) #51797

Closed

dgieselaar mentioned this pull request Nov 28, 2019

[APM] Service Map data background task and API endpoint #50844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks #50232

[Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks #50232

gmmorris commented Nov 11, 2019 •

edited

Loading

elasticmachine commented Nov 11, 2019

elasticmachine commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

gmmorris commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

elasticmachine commented Nov 13, 2019

elasticmachine commented Nov 13, 2019

mikecote commented Nov 14, 2019

gmmorris commented Nov 14, 2019

bmcconaghy left a comment

pmuellr left a comment

pmuellr Nov 14, 2019

gmmorris Nov 14, 2019

pmuellr Nov 14, 2019

gmmorris Nov 14, 2019

pmuellr commented Nov 14, 2019

gmmorris commented Nov 14, 2019

pmuellr Nov 14, 2019

wylieconlon Nov 14, 2019

wylieconlon left a comment

gmmorris commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

[Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks #50232

[Task manager] Adds ensureScheduled api to allow safer rescheduling of existing tasks #50232

Conversation

gmmorris commented Nov 11, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 11, 2019

elasticmachine commented Nov 12, 2019

💔 Build Failed

elasticmachine commented Nov 12, 2019

💔 Build Failed

elasticmachine commented Nov 12, 2019

💔 Build Failed

gmmorris commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

💔 Build Failed

elasticmachine commented Nov 12, 2019

💔 Build Failed

elasticmachine commented Nov 13, 2019

💚 Build Succeeded

elasticmachine commented Nov 13, 2019

💚 Build Succeeded

mikecote commented Nov 14, 2019

gmmorris commented Nov 14, 2019

bmcconaghy left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Nov 14, 2019

Choose a reason for hiding this comment

gmmorris Nov 14, 2019

Choose a reason for hiding this comment

pmuellr Nov 14, 2019

Choose a reason for hiding this comment

gmmorris Nov 14, 2019

Choose a reason for hiding this comment

pmuellr commented Nov 14, 2019

gmmorris commented Nov 14, 2019

pmuellr Nov 14, 2019

Choose a reason for hiding this comment

wylieconlon Nov 14, 2019

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

gmmorris commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

gmmorris commented Nov 11, 2019 •

edited

Loading