Skip to content

Code cleanup in TransportRolloverAction and added concurrent rollover test. #80397

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 7 commits into from
Nov 8, 2021
Merged

Code cleanup in TransportRolloverAction and added concurrent rollover test. #80397

merged 7 commits into from
Nov 8, 2021

Conversation

@martijnvg
Copy link
Member

@martijnvg martijnvg commented Nov 5, 2021 •
edited
Loading

Code cleanups around rollover executor in TransportRolloverAction and added an integration test that tests rollover concurrently.

Relates to #79945

@martijnvg martijnvg added >non-issue :Data Management/Indices APIs APIs to create and manage indices and templates v8.0.0 v8.1.0 labels Nov 5, 2021
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Nov 5, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 5, 2021

Pinging @elastic/es-data-management (Team:Data Management)

henningandersen
henningandersen reviewed Nov 5, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

Thanks for improving this. Can we also add more testing, to check that the batched case works (perhaps just an integ test doing X rollovers concurrently)?

@martijnvg
Copy link
Member Author

martijnvg commented Nov 5, 2021

@henningandersen I've added an integration test: 3ac70fa

@dakrone
Copy link
Member

dakrone commented Nov 5, 2021

@martijnvg can you update the title and description of this PR so we can tell what it's doing? I want to make sure we can find it in the future using keywords for what this PR is actually changing.

@martijnvg martijnvg changed the title Addressed followups from batch rollover PR. Code cleanup in TransportRolloverAction and added concurrent rollover test. Nov 5, 2021
@martijnvg
Copy link
Member Author

martijnvg commented Nov 5, 2021

Can we add a CyclicBarrier(numOfThreads) that ensures all threads start the rollovers at the same time, just to increase the chance of failure.

@henningandersen I've added this here: 829cd2d

henningandersen
henningandersen approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing this.

@martijnvg
Copy link
Member Author

martijnvg commented Nov 5, 2021

@elasticmachine update branch

@martijnvg
Copy link
Member Author

martijnvg commented Nov 5, 2021

LGTM, thanks for addressing this.

Thanks for bringing this up!

@martijnvg martijnvg merged commit 3592aed into elastic:master Nov 8, 2021
@martijnvg martijnvg mentioned this pull request Nov 8, 2021
Merged
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 8, 2021
@martijnvg
Code cleanup in TransportRolloverAction and added concurrent rollover…
88f3b8a 
… test. (elastic#80397)

Code cleanups around rollover executor in TransportRolloverAction and added an integration test that tests rollover concurrently.

Relates to elastic#79945
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Nov 8, 2021

💚 Backport successful

Status Branch Result
✅ 8.0

elasticsearchmachine pushed a commit that referenced this pull request Nov 8, 2021
@martijnvg @elasticmachine
Code cleanup in TransportRolloverAction and added concurrent rollover…
7e8130d 
… test. (#80397) (#80478)

Code cleanups around rollover executor in TransportRolloverAction and added an integration test that tests rollover concurrently.

Relates to #79945

Co-authored-by: Elastic Machine <[email protected]>
@pugnascotia pugnascotia removed the v8.0.0 label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningandersen henningandersen henningandersen approved these changes

Assignees

No one assigned

Labels

:Data Management/Indices APIs APIs to create and manage indices and templates >non-issue Team:Data Management Meta label for data/management team v8.0.0-rc2 v8.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@martijnvg @elasticmachine @dakrone @elasticsearchmachine @henningandersen @pugnascotia