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

Removing unnecessary state from DataStreamReindexTask #117942

Conversation

masseyke
Copy link
Member

@masseyke masseyke commented Dec 3, 2024

DataStreamReindexTask currently hods onto lists of index names that are being processed. This is unnecessary for the status we are reporting, and overcomplicates things. This small cleanup PR changes those to counts instead. It also removes a no-longer-used ThreadPool reference.

@masseyke masseyke requested a review from dakrone December 3, 2024 23:16
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Dec 3, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@dakrone
Copy link
Member

dakrone commented Dec 3, 2024

How did we decide that the indices being affected aren't as important? (Just out of curiosity, not trying to undo this)

@masseyke
Copy link
Member Author

masseyke commented Dec 3, 2024

How did we decide that the indices being affected aren't as important? (Just out of curiosity, not trying to undo this)

This information will be used for reporting the status of the persistent task. We're trying not to expose backing index information in status -- I think we (the upgrade assistant) would rather report it as how much of the data stream is complete, without ever mentioning index names. Also, if we were to report index names we'd need to record each completed index in the cluster state, which would add to the burden on the master node without buying us anything. This way we just record the total number at the beginning of the task, and can compute how many are pending or in progress or completed on the fly.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

public void setInProgressIndices(List<String> inProgressIndices) {
this.inProgress = inProgressIndices;
public void reindexSucceeded() {
inProgress.decrementAndGet();
Copy link
Member

Choose a reason for hiding this comment

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

We should add asserts (in the future) that these decrementing actions never go negative

@masseyke masseyke added the auto-backport Automatically create backport pull requests when merged label Dec 4, 2024
@masseyke masseyke merged commit ec27e20 into elastic:main Dec 4, 2024
16 checks passed
@masseyke masseyke deleted the removing-unnecessary-state-from-DataStreamReindexTask branch December 4, 2024 13:38
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Dec 4, 2024
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

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 :Data Management/Data streams Data streams and their lifecycles >refactoring Team:Data Management Meta label for data/management team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants