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

[ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch #102506

Merged
merged 2 commits into from
Jun 18, 2021
Merged

[ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch #102506

merged 2 commits into from
Jun 18, 2021

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented Jun 17, 2021

Summary

This a companion to elastic/elasticsearch#74188.

I think this PR is effectively a no-op, as the modified
method is never called. But it seems sensible to remove
it now that it's out-of-sync with Elasticsearch, to prevent
it being used in the future.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

This a companion to elastic/elasticsearch#74188.

I _think_ this PR is effectively a no-op, as the modified
method is never called. But it seems sensible to remove
fields that no longer exist from it, just in case it ever
is called in the future.
@droberts195 droberts195 added :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 17, 2021
@droberts195 droberts195 requested a review from a team as a code owner June 17, 2021 13:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@jgowdyelastic
Copy link
Member

Indeed this function is old and never called. This whole file is contains very old utility functions for job creation and management which should be removed and the rest of the code converted to typescript.
We have some old code in the anomaly explorer and single metric viewer which relies on the jobs loaded by this file, but they should be refactored to remove this dependency.

It would be fine for this PR to go in, or to be changed to remove this getBlankJob function entirely.

@jgowdyelastic
Copy link
Member

The esclient should be updated to remove field_delimiter
https://github.com/elastic/elasticsearch-specification/blob/05822d61fa83bd1650f62bb9758cb8134af5f616/specification/ml/_types/Job.ts#L123

is format still needed if it is always xcontent?

@droberts195
Copy link
Contributor Author

It would be fine for this PR to go in, or to be changed to remove this getBlankJob function entirely.

OK, I will change it to remove the function entirely.

The esclient should be updated to remove field_delimiter

I told the clients team about that in elastic/elasticsearch#74188 (comment).

@droberts195 droberts195 changed the title [ML] Adjust blank job definition to reflect Elasticsearch changes [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch Jun 17, 2021
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 5.9MB 5.9MB -227.0B

History

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

@droberts195 droberts195 merged commit e518865 into elastic:master Jun 18, 2021
@droberts195 droberts195 deleted the adjust_blank_ml_job branch June 18, 2021 09:49
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 18, 2021
…ets-tab

* 'master' of github.com:elastic/kibana: (93 commits)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  [Maps] clean up feature editing name space to avoid conflicts with layer settings editing (elastic#102516)
  [canvas] Refactor Storybook from bespoke to standard configuration (elastic#101962)
  [Security Solution] adds wrapSequences method (RAC) (elastic#102106)
  [FTR] Stabilize SSLP functional tests (elastic#102553)
  [K8] Added `Inter` font files for new theme (elastic#102359)
  [Workplace Search] Convert Groups pages to new page template (elastic#102449)
  [DOC] Add experimental disclaimer to rollup jobs (elastic#95624)
  [Security Solution][Endpoint] Suppress some of the jest console.error noise created by endpoint list middelware (elastic#102535)
  [Fleet] Improve performance of Fleet setup (elastic#102219)
  [Alerting] Add event log entry when a rule starts executing (elastic#102001)
  [Fleet] Update docker image of registry used in integration tests (elastic#101911)
  [Asset Management] Osquery telemetry updates (elastic#100754)
  Converts saved object tagging to new management layout (elastic#102284)
  ...

# Conflicts:
#	x-pack/plugins/fleet/kibana.json
@droberts195 droberts195 added auto-backport Deprecated - use backport:version if exact versions are needed v7.14.0 labels Jun 18, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 18, 2021
… Elasticsearch (elastic#102506)

This a companion to elastic/elasticsearch#74188.

This PR is functionally a no-op, as the removed method
was not called anywhere. But it is sensible to remove
it to prevent it being called in the future now that it
references fields that don't exist in Elasticsearch.
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 18, 2021
… Elasticsearch (#102506) (#102620)

This a companion to elastic/elasticsearch#74188.

This PR is functionally a no-op, as the removed method
was not called anywhere. But it is sensible to remove
it to prevent it being called in the future now that it
references fields that don't exist in Elasticsearch.

Co-authored-by: David Roberts <dave.roberts@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 21, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (447 commits)
  skip flaky suite (elastic#102366)
  [Security Solution][Endpoint][Host Isolation] Isolation status badge from alert details (elastic#102274)
  Add email connector info for Elastic Cloud (elastic#91363)
  [Workplace Search] remove or replace xs props for text on source connect view (elastic#102663)
  Do not double register dashboard url generator (elastic#102599)
  [TSVB] Replaces EuiCodeEditor 👉 Monaco editor  (elastic#100684)
  [Discover] Update kibana.json adding owner and description (elastic#102292)
  [Exploratory View] Mobile experience (elastic#99565)
  chore(NA): moving @kbn/ui-shared-deps into bazel (elastic#101669)
  [TSVB] Index pattern select field disappear in Annotation tab (elastic#102314)
  [Security Solution][Endpoint][Host Isolation] Fixes bug where host isolation/unisolation works from alert details (elastic#102581)
  TSVB visualizations with no timefield do not render after upgrading from 7.12.1 to 7.13.0 (elastic#102494)
  [Logs UI] Add `event.original` fallback to message reconstruction rules (elastic#102236)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Anomaly Detection ML anomaly detection :ml release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants