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

[file_upload] move ml Importer classes to file_upload plugin #91559

Merged
merged 16 commits into from
Feb 22, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Feb 16, 2021

This PR moves ml Importer classes to file_upload plugin. The PR then refactors geojson upload into a GeoJsonImporter.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@nreese nreese requested a review from a team as a code owner February 17, 2021 15:24
@nreese
Copy link
Contributor Author

nreese commented Feb 17, 2021

@elasticmachine merge upstream

@kindsun
Copy link
Contributor

kindsun commented Feb 17, 2021

Just wanted to comment to say I've done a first pass over the code and tested several files on the Maps side. No glaring errors and things appear to be working well! Still planning at least a second deeper dive, but overall nice work 👍

@nreese
Copy link
Contributor Author

nreese commented Feb 18, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Feb 19, 2021

@elasticmachine merge upstream

@nreese nreese requested a review from kindsun February 19, 2021 13:56
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.

Added a couple of comments, but on the whole LGTM.
Nice work!

Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Overall the Maps (GeoJSON) portion lgtm! This PR retains the parts we really want to keep: primarily the UI and parsing logic; and removes the parts we don't need and can easily share in a consolidated plugin: primarily indexing and chunking.

  • Tested locally in chrome with various test GeoJSON files
  • Code review

Nice work! 💯

@nreese
Copy link
Contributor Author

nreese commented Feb 22, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fileUpload 189 197 +8
ml 1740 1735 -5
total +3

Async chunks

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

id before after diff
fileUpload 825.2KB 826.0KB +812.0B
maps 2.6MB 2.6MB -282.0B
ml 6.4MB 6.3MB -6.5KB
total -6.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fileUpload 10.4KB 11.5KB +1.1KB
ml 68.0KB 68.3KB +303.0B
total +1.4KB

History

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

@nreese nreese merged commit 6ef7d2c into elastic:master Feb 22, 2021
nreese added a commit to nreese/kibana that referenced this pull request Feb 22, 2021
…#91559)

* [ml] move importer to file_upload plugin

* move file_parse logic into GeoJsonImporter

* move file_parser tests to geojson_importer tests

* rename geo_json_clean_and_validate to geojson_clean_and_validate

* replace file_upload import with Importer.import

* simplify JsonIndexFilePicker props

* tslint

* i18n fixes and tslint fixes

* update functional test to account for change in layer name

* review feedback

* dependency_cache review feedback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 22, 2021
* master:
  Ability to filter alerts by string parameters (elastic#92036)
  [APM] Fix for flaky correlations API test (elastic#91673) (elastic#92094)
  [Enterprise Search] Migrate shared role mapping components (elastic#91723)
  [file_upload] move ml Importer classes to file_upload plugin (elastic#91559)
  [Discover] Always show the "hide missing fields" toggle (elastic#91889)
  v2 migrations should exit process on corrupt saved object document (elastic#91465)
  [ML] Data Frame Analytics exploration page: filters improvements (elastic#91748)
  [ML] Data Frame Analytics: Improved error handling for scatterplot matrix. (elastic#91993)
  [coverage] speed up merging results of functional tests (elastic#92111)
  Adds a Reason indicator to the onClose handler in AddAlert and EditAlert (elastic#92149)
nreese added a commit that referenced this pull request Feb 22, 2021
…#92213)

* [ml] move importer to file_upload plugin

* move file_parse logic into GeoJsonImporter

* move file_parser tests to geojson_importer tests

* rename geo_json_clean_and_validate to geojson_clean_and_validate

* replace file_upload import with Importer.import

* simplify JsonIndexFilePicker props

* tslint

* i18n fixes and tslint fixes

* update functional test to account for change in layer name

* review feedback

* dependency_cache review feedback

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants