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

[Maps] [File upload] Fix maps geojson upload hanging on index step #42623

Merged
merged 6 commits into from
Aug 8, 2019

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Aug 5, 2019

Resolves #40102. Indexing would hang when requests exceeded the allotted timeout of 10 seconds, at which point fetchWithTimeout would return null, causing downstream issues. This PR makes a couple of changes:

  1. Remove timeout logic. We're already ultimately bound by the sever-side cluster index call callWithRequest which times out after 30 seconds
  2. Return object with a consistent failures shape. Since the client can make multiple fetch calls after chunking up the data, it makes the most sense to return an object with a consistent shape for easy gathering of failures across multiple fetches.
  3. Handle the case where the index request was a success but all docs were failures. The maps app currently allows doc failures, but it doesn't make sense to add a layer where all docs failed since there is no data to add.

Attached are two files used for test. The indexing of the larger of the two files, addresses.geo.json exceeded the previous timeout of 10 seconds on account of using a reserved field _id in all docs. Previously this behavior resulted in the issue described, now it times out on the server side and the message is sent back to the client. The smaller file short_addresses.geo.json doesn't exceed any timeouts but is returned a success with all docs failed. This is correctly handled as a non-addable layer.

long_and_short_addresses_test_files.tar.gz

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kindsun
Copy link
Contributor Author

kindsun commented Aug 6, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kindsun kindsun requested a review from thomasneirynck August 6, 2019 19:56
@thomasneirynck
Copy link
Contributor

A good test for this one woudl be the file linked here: https://discuss.elastic.co/t/custom-vector-maps-with-accents/194075/3

It's a small-ish file, with smallish number of features (a few thousand), but large-ish geometries.

Can you upload this file now after this PR?

@kindsun
Copy link
Contributor Author

kindsun commented Aug 8, 2019

A good test for this one woudl be the file linked here: https://discuss.elastic.co/t/custom-vector-maps-with-accents/194075/3

It's a small-ish file, with smallish number of features (a few thousand), but large-ish geometries.

Can you upload this file now after this PR?

Nice dataset. Uploaded without issue!

image

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck self-requested a review August 8, 2019 18:17
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

See comment to clean up redundant branching and trim some LOC/tmp vars.

Geojson is getting real nice with all the recent bug fixes. We're getting to upload some real-world datasets without a hitch. 👏

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kindsun kindsun merged commit 6335a12 into elastic:master Aug 8, 2019
kindsun pushed a commit to kindsun/kibana that referenced this pull request Aug 8, 2019
…lastic#42623)

* Remove timeout from fetch. Falls back to 30 second timeout on server-side indexing call

* Handle when indexing request is a success but every doc failed

* Return failures array from catch

* Review feedback

* Review feedback
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (306 commits)
  [ML] Adding job overrides to the module setup endpoint (elastic#42946)
  [APM] Fix missing RUM url (elastic#42940)
  close socket timeouts without message (elastic#42456)
  Upgrade elastic/charts to 8.1.6 (elastic#42518)
  [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962)
  Add custom formatting for Date Nanos Format (elastic#42445)
  [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582)
  add socket.getPeerCertificate to KibanaRequest (elastic#42929)
  [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683)
  [ML] Data frames: Updated stats structure. (elastic#42923)
  [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841)
  [kbn-es] Support for passing regex value to ES (elastic#42651)
  Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840)
  Add Elasticsearch SSL support for integration tests (elastic#41765)
  Fix duplicate fetch in Visualize (elastic#41204)
  [DOCS] TSVB and Timelion clean up (elastic#42953)
  [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623)
  [APM] Use rounded bucket sizes for transaction distribution (elastic#42830)
  [yarn.lock] consistent resolve domain (elastic#42969)
  [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650)
  ...
kindsun pushed a commit to kindsun/kibana that referenced this pull request Aug 9, 2019
…lastic#42623)

* Remove timeout from fetch. Falls back to 30 second timeout on server-side indexing call

* Handle when indexing request is a success but every doc failed

* Return failures array from catch

* Review feedback

* Review feedback

# Conflicts:
#	x-pack/legacy/plugins/maps/public/layers/sources/client_file_source/geojson_file_source.js
kindsun pushed a commit that referenced this pull request Aug 9, 2019
…42623) (#42983)

* Remove timeout from fetch. Falls back to 30 second timeout on server-side indexing call

* Handle when indexing request is a success but every doc failed

* Return failures array from catch

* Review feedback

* Review feedback
kindsun pushed a commit that referenced this pull request Aug 9, 2019
…tep (#42623) (#43033)

* [Maps] [File upload] Fix maps geojson upload hanging on index step (#42623)

* Remove timeout from fetch. Falls back to 30 second timeout on server-side indexing call

* Handle when indexing request is a success but every doc failed

* Return failures array from catch

* Review feedback

* Review feedback

# Conflicts:
#	x-pack/legacy/plugins/maps/public/layers/sources/client_file_source/geojson_file_source.js

* Indentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Geojson upload hangs with large files at the writing to index step
3 participants