Skip to content
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

[Reporting] remove unused "immediate export" abstractions from esqueue worker cycles #65375

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 5, 2020

Summary

Moving the Reporting plugin's job management from ESQueue to Task Manager means first simplifying parts of the Reporting code that deal with queuing job (scheduling task) and executing the job (running the task).

Previous to this PR, there was an overcomplication in this parts of the code, because an unnecessary abstraction existed that added awareness of types of tasks that should execute immediately, instead of being queued.

It turns out those abstractions were not needed, because immediate task work doesn't come through this part of the code.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

More testing on the affected feature is added here: #65401

@tsullivan tsullivan force-pushed the reporting/dashboard-csv-download-maintainance branch from ba4babd to 5fe0845 Compare May 5, 2020 21:43
@tsullivan tsullivan changed the title remove immediate functions from esqueue worker cycles [Reporting] remove unused abstractions from esqueue worker cycles May 5, 2020
@tsullivan tsullivan changed the title [Reporting] remove unused abstractions from esqueue worker cycles [Reporting] remove unused "immediate export" abstractions from esqueue worker cycles May 6, 2020
@tsullivan tsullivan force-pushed the reporting/dashboard-csv-download-maintainance branch from 564605a to b475b4c Compare May 6, 2020 00:07
@tsullivan tsullivan force-pushed the reporting/dashboard-csv-download-maintainance branch from b475b4c to ac34f92 Compare May 6, 2020 00:11
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v7.9.0 v8.0.0 labels May 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@tsullivan tsullivan marked this pull request as ready for review May 7, 2020 16:15
@elastic elastic deleted a comment from elasticmachine May 7, 2020
@elastic elastic deleted a comment from elasticmachine May 7, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan
Copy link
Member Author

This PR is entirely Typescript maintenance in 2 files. The original code seems over-eager to support a type of data (export type with different signature in the create/execute functions) which turns out to not be relevant here because immediate report generation is indeed handled outside of the queue/execute cycles.

Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM!

@tsullivan tsullivan merged commit 9b71f46 into elastic:master May 7, 2020
@tsullivan tsullivan deleted the reporting/dashboard-csv-download-maintainance branch May 7, 2020 19:07
tsullivan added a commit to tsullivan/kibana that referenced this pull request May 7, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 8, 2020
…or-part-mvp-2

* 'master' of github.com:elastic/kibana: (259 commits)
  SavedObjects bulkCreate API should return migrationVersion and strip the type & namespace from the id (elastic#65150)
  Drilldown count tooltip (elastic#65105)
  plugins logs start with "plugins." prefix (elastic#65710)
  [ML] Fix pagination reset on search query update. (elastic#65668)
  [SIEM] Add types to the mappings objects so extra keys cannot be introduced
  [apm] Update machine learning flyout and service maps docs (elastic#65517)
  change api endpoint and throw error (elastic#65790)
  [Maps] remove SLA percentage metric (elastic#65718)
  [Reporting] APM integration for baseline performance measurements (elastic#59967)
  fix(NA): noParse regex for windows on kbn optimizer (elastic#65755)
  [ML] DFA: ensure at least one field is included in analysis before job can be created (elastic#65320)
  [Data plugin] cleanup - remove unused getRoutes / routes from indexPattern object (elastic#65683)
  Removed skip to enable test. (elastic#65575)
  [Lens] Type safe migrations (elastic#65576)
  [Canvas] Fix nav link behavior in Canvas  (elastic#65590)
  [Event log] Fix flaky test (elastic#65658)
  [Alerting] changes preconfigured actions config from array to object (elastic#65397)
  remove immediate functions from esqueue worker cycles (elastic#65375)
  [Metrics UI] Fix isAbove to only display when threshold set (elastic#65540)
  draft search profiler accessibility tests (elastic#62357)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
tsullivan added a commit that referenced this pull request May 8, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants