add default client module, more wrapper methods#3
Conversation
tsullivan
force-pushed
the
alerting/task-scheduler-return-taskdoc-to-caller
branch
from
ae3c66a to
b893db7
Compare
chrisdavies
left a comment
chrisdavies
left a comment
There was a problem hiding this comment.
LGTM w/ tweaks, particularly validating the client in the constructor and / or setter, rather than in the points where it's consumed.
| public schedule(task: TaskInstance) { | ||
| if (this.client == null) { | ||
| throw new Error('Task Manager Client has not been set properly!'); | ||
| const { error } = checkClient(this.client); |
There was a problem hiding this comment.
I think instead of putting this check in each method, we should just have a sane default client, and put this check in any place that sets the client.
| this.poller.attemptWork(); | ||
| public async schedule(task: TaskInstance): Promise<RawTaskDoc> { | ||
| const result = await this.store.schedule(task); | ||
| this.poller.attemptWork(); // TODO await this? |
There was a problem hiding this comment.
I don't think it's necessary to await this, as the attemptWork won't fail (it swallows / logs errors). And there's no reason to further delay the call to schedule.
- setClient takes a callback fn
This is a great suggestion, and I addressed in 3a7a5c2 Having a helper module that constructs the default client turns out to be very helpful, I think, to see how an outside plugin (security) can construct its own client without having to do the "setup" work like scanning for task definitions and registering a logger. We talked about that need before, and I am addressing it here by making |
|
7482c0d and 48487b5 were things I was getting from warnings in the existing code, give those a good look too. |
| pool.run, | ||
| store.fetchAvailableTasks, | ||
| (instance: ConcreteTaskInstance) => | ||
| new TaskManagerRunner({ |
There was a problem hiding this comment.
I moved this to the default client, and reordered the fields to be in the order that TaskManagerRunner defines. VS Code was giving a warning about this before & after I touched it
tsullivan
changed the title
tsullivan
changed the title
- setClient takes a callback fn
…cheduler-return-taskdoc-to-caller
…ask-scheduler-return-taskdoc-to-caller
tsullivan
deleted the
alerting/task-scheduler-return-taskdoc-to-caller
branch
* Prepare control flow to use embeddable factories in add panel * Rewrite saved object finder and add tests * Fix usages of new saved object finder * fix test failures * fix some functional tests and re-introduce makeUrl * fix tests * remove direct hrefs in saved_object_lists * PR review fixes * update snapshot * overwrite width of viz dialog * Update src/legacy/core_plugins/kibana/public/dashboard/top_nav/add_panel.js Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable_factory.ts Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/discover/top_nav/open_search_panel.js Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/visualize/wizard/search_selection/search_selection.tsx Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/visualize/wizard/search_selection/search_selection.tsx Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/visualize/wizard/search_selection/search_selection.tsx Co-Authored-By: flash1293 <email@johannes-reuter.de> * fix tests * review fixes #1 * review fixes #2 * dont use classname in functional test * remove call to action button prop * align buttons correctly * fix tests * remove debugging statement * Update src/legacy/core_plugins/kibana/public/dashboard/top_nav/add_panel.js Co-Authored-By: flash1293 <email@johannes-reuter.de> * Update src/legacy/core_plugins/kibana/public/discover/top_nav/open_search_panel.js Co-Authored-By: flash1293 <email@johannes-reuter.de> * review fixes #3 * improve filter behavior and enable it for search wizard * adjust functional tests for new filter behavior * Change translation id due to string change * Update Jest snapshot
2 participants
These changes give access to plugins the task ID and other information, which is needed for removing tasks and getting other info.