Skip to content

Rollup performance improvement: index bucket replace map with smile builder #89495

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 9, 2022
Merged

Rollup performance improvement: index bucket replace map with smile builder #89495

merged 3 commits into from
Sep 9, 2022

Conversation

weizijun
Copy link
Contributor

index bucket replace map with smile builder.
It can greatly improve the build source and index api performance.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.5.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 22, 2022
@DJRickyB DJRickyB added :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data and removed needs:triage Requires assignment of a team area label labels Aug 31, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 31, 2022
@csoulios csoulios self-requested a review September 7, 2022 13:14
@csoulios csoulios self-assigned this Sep 7, 2022
@csoulios
Copy link
Contributor

csoulios commented Sep 7, 2022

@elasticmachine ok to test

csoulios
csoulios reviewed Sep 7, 2022
View reviewed changes
Copy link
Contributor

@csoulios csoulios left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This change theoretically improves indexing performance.
Have you benchmarked its impact? Can you share any numbers?

Also, can you please merge from upstream and resolve conflicts?

Thanks

@weizijun
Merge branch 'main' into rollup-use-smile-to-replace-map
ba10533 
* main: (175 commits)
  Fix integration test on Windows (elastic#89894)
  Avoiding the use of dynamic map keys in the cluster_formation results of the stable master health indicator (elastic#89842)
  Mute org.elasticsearch.tracing.apm.ApmIT.testCapturesTracesForHttpTraffic (elastic#89891)
  Fix typos in audit event types (elastic#89886)
  Synthetic _source: support histogram field (elastic#89833)
  [TSDB] Rename rollup public API to downsample  (elastic#89809)
  Format script values access (elastic#89780)
  [DOCS] Simplifies composite aggregation recommendation (elastic#89878)
  [DOCS] Update CCS compatibility matrix for 8.3 (elastic#88906)
  Fix memory leak when double invoking RestChannel.sendResponse (elastic#89873)
  [ML] Add processor autoscaling decider (elastic#89645)
  Update disk-usage.asciidoc (elastic#89709) (elastic#89874)
  Add allocation deciders in createComponents (elastic#89836)
  Mute flaky H3LatLonGeometryTest.testIndexPoints (elastic#89870)
  Fix typo in get-snapshot-status-api doc (elastic#89865)
  Picking master eligible node at random in the master stability health indicator (elastic#89841)
  Do not reuse the client after a disruption elastic#89815 (elastic#89866)
  [ML] Distribute trained model allocations across availability zones (elastic#89822)
  Increment clientCalledCount before onResponse (elastic#89858)
  AwaitsFix for elastic#89867
  ...

# Conflicts:
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java
@weizijun
Merge branch 'main' into rollup-use-smile-to-replace-map
b668c2f 
* main: (34 commits)
  Make sure ivy repo directory exists before downloading artifacts
  Use 'file://' scheme for local repository URL
  Use DRA artifacts for release build CI jobs
  Log unsuccessful attempts to get credentials from web identity tokens (elastic#88241)
  Script: Write Field API path manipulation (elastic#89889)
  Fetch health info action (elastic#89820)
  Fix memory leak in TransportDeleteExpiredDataAction (elastic#89935)
  [ML] Performance improvements for categorization jobs (elastic#89824)
  [DOCS] Revert changes for ES_JAVA_OPTS (elastic#89931)
  Fix deadlock bug exposed by a test (elastic#89934)
  [Downsampling] Remove `FieldValueFetcher` validator (elastic#89497)
  Fix segment stats in tsdb (elastic#89754)
  Synthetic _source: support dense_vector (elastic#89840)
  REST tests fetching fields with synthetic _source (elastic#89888)
  Do not deserialize back BytesTransportRequest to clone a request in MockTransportService (elastic#89926)
  Add SDK request logging to debug failures of S3BlobStoreRepositoryTests#testRequestStats (elastic#89912)
  Fix SnapshotStatusApisIT.testGetSnapshotsWithSnapshotInProgress (elastic#89925)
  Document synthetic source for text and keyword (elastic#89893)
  Fix CloneSnapshotIT.testRemoveFailedCloneFromCSWithQueuedSnapshotInProgress (elastic#89914)
  Add missing index.mapping.total_fields.limit setting to the target index (elastic#89875)
  ...
@weizijun
Copy link
Contributor Author

weizijun commented Sep 9, 2022

before:
20220909-155830

after:
20220909-164216

In my benchmark, the downsampling cost from 2.4 minute to 2.2 minute.

@csoulios
Copy link
Contributor

csoulios commented Sep 9, 2022

@weizijun thanks for posting the graphs and results.

It is worth noting the downsampling interval as well as the ratio of source_index_doc_count/rollup_index_doc_count.

We deem the downsampling operation spends more time in reading the source index than writing to the rollup index. So, improving the write part of the operation will yield small improvements compared to improving the read part.

However, this change makes perfect sense and we plan to merge it.

csoulios
csoulios approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM!

@csoulios csoulios merged commit 77c9e2d into elastic:main Sep 9, 2022
@weizijun
Copy link
Contributor Author

weizijun commented Sep 9, 2022

@weizijun thanks for posting the graphs and results.

It is worth noting the downsampling interval as well as the ratio of source_index_doc_count/rollup_index_doc_count.

We deem the downsampling operation spends more time in reading the source index than writing to the rollup index. So, improving the write part of the operation will yield small improvements compared to improving the read part.

However, this change makes perfect sense and we plan to merge it.

Thanks @csoulios !

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Sep 9, 2022
@weizijun
Merge branch 'main' into fixed-parse-tsid
1b2d52b 
* main: (176 commits)
  Fix RandomSamplerAggregatorTests testAggregationSamplingNestedAggsScaled test failure (elastic#89958)
  [Downsampling] Replace document map with SMILE encoded doc (elastic#89495)
  Remove full cluster state from error logging in MasterService (elastic#89960)
  [ML] Truncate categorization fields (elastic#89827)
  [TSDB] Removed `summary` and `histogram` metric types (elastic#89937)
  Update testNodeSelectorRouting so that it does not depend on iteration order (elastic#89879)
  Make sure listener is resolved when file queue is cleared (elastic#89929)
  [Stable plugin api] Extensible annotation (elastic#89903)
  Fix double sending of response in TransportOpenIdConnectPrepareAuthenticationAction (elastic#89930)
  Make sure ivy repo directory exists before downloading artifacts
  Use 'file://' scheme for local repository URL
  Use DRA artifacts for release build CI jobs
  Log unsuccessful attempts to get credentials from web identity tokens (elastic#88241)
  Script: Write Field API path manipulation (elastic#89889)
  Fetch health info action (elastic#89820)
  Fix memory leak in TransportDeleteExpiredDataAction (elastic#89935)
  [ML] Performance improvements for categorization jobs (elastic#89824)
  [DOCS] Revert changes for ES_JAVA_OPTS (elastic#89931)
  Fix deadlock bug exposed by a test (elastic#89934)
  [Downsampling] Remove `FieldValueFetcher` validator (elastic#89497)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csoulios csoulios csoulios approved these changes

Assignees

@csoulios csoulios

Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@weizijun @elasticsearchmachine @csoulios @DJRickyB