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

[Transform][Rollup] remove unnecessary list indirection #75459

Merged
merged 4 commits into from
Jul 28, 2021

Conversation

hendrikmuhs
Copy link

Remove an unnecessary indirection and refactor progress tracking. Both rollup and transform process documents as stream, however in the AsyncTwoPhaseIndexer takes a List of index requests. This change removes the unnecessary temporary container. This simplification makes upcoming transform enhancements easier.

Main changes are in 15afd31
The 2nd commit only applies spotless formatting to files belonging to the transform codebase.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@hendrikmuhs hendrikmuhs added the :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data label Jul 19, 2021
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 19, 2021
@elasticmachine
Copy link
Collaborator

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

@hendrikmuhs
Copy link
Author

@elasticmachine update branch

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Overall, I like the idea, it does simplify execution paths.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

The stream stuff seems like a good choice to me. It looks like it doesn't close over anything sneaky - just the response object which we keep around anyway.

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

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

@hendrikmuhs hendrikmuhs force-pushed the stream-indexrequests branch from 3484f66 to 0cb6c0a Compare July 27, 2021 12:36
@hendrikmuhs hendrikmuhs added the auto-backport Automatically create backport pull requests when merged label Jul 28, 2021
@hendrikmuhs hendrikmuhs merged commit fb0846a into elastic:master Jul 28, 2021
@hendrikmuhs hendrikmuhs deleted the stream-indexrequests branch July 28, 2021 14:34
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
backport --pr 75459

hendrikmuhs pushed a commit that referenced this pull request Jul 29, 2021
#75829)

Remove an unnecessary indirection and refactor progress tracking. Both rollup and transform
process documents as stream, however in the AsyncTwoPhaseIndexer takes a List of index
requests. This change removes the unnecessary temporary container and makes upcoming
transform enhancements easier.
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
Remove an unnecessary indirection and refactor progress tracking. Both rollup and transform
process documents as stream, however in the AsyncTwoPhaseIndexer takes a List of index
requests. This change removes the unnecessary temporary container and makes upcoming
transform enhancements easier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :ml/Transform Transform >refactoring :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:ML Meta label for the ML team v7.15.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants