Skip to content

Stop MAC servers in parallel - #6575

Merged
DomGarguilo merged 2 commits into
apache:2.1from
DomGarguilo:ParallelstopProcs
Oct 8, 2026
Merged

DomGarguilo merged 2 commits into
apache:2.1from
DomGarguilo:ParallelstopProcs

Conversation

@DomGarguilo

Copy link
Copy Markdown
Member

Looking through code paths that could help speed up the ITs and found this one.

In the old code, MiniAccumuloClusterControl.stop() stops tservers, sservers and compactors one at a time and waits after stopping each. The new code now sends the SIGTERM to all processes of that server type first, then waits on each. This allows them all to start shutting down in parallel before the wait step.

I did some rough testing by timing stop() and found that this speeds things up by ~350ms per server which is negligible on a single run but the time savings multiply.

@DomGarguilo DomGarguilo added this to the 2.1.7 milestone Oct 1, 2026
@DomGarguilo DomGarguilo self-assigned this Oct 1, 2026
@dlmarion

dlmarion commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

It looks like we are waiting up to 30 seconds per server even with your change.

            for (Process tserver : tabletServerProcesses) {
              try {
                cluster.stopProcessWithTimeout(tserver, 30, TimeUnit.SECONDS);
              } catch (ExecutionException | TimeoutException e) {
             ...
           }

I wonder if instead we change the signature of cluster.stopProcessWithTimeout to accept all processes, incorporate your change in the that method, and wait a total of 30s for all processes to stop instead of one.

Thoughts?

@DomGarguilo

DomGarguilo commented Oct 1, 2026 •

Copy link
Copy Markdown
Member Author

Yea I think there are some options here. The changes here keep the same order in which servers are shut down. We could try shutting all processes down at once but that would obviously not guarantee shutdown order which might be needed for some reason in certain tests.

Its worth mentioning that the 30s wait is just a worst case scenario but it is per process so if all processes fail to stop properly we would be waiting 30s for each.

@dlmarion

dlmarion commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

The changes here keep the same order in which servers are shut down. We could try shutting all processes down at once but that would obviously not guarantee shutdown order which might be needed for some reason in certain tests.

I'm not sure I'm following w/r/t the order. The changes are in a switch statement based on the server method parameter. So, in this method, we are only shutting down one type of server. I'm just suggesting that instead of this method calling cluster.stopProcessWithTimeout(tserver, 30, TimeUnit.SECONDS); it calls cluster.stopProcessWithTimeout(tabletServerProcesses, 30, TimeUnit.SECONDS); instead and inside of stopProcessWithTimeout we shut them all down in parallel.

@DomGarguilo

Copy link
Copy Markdown
Member Author

I'm not sure I'm following w/r/t the order. The changes are in a switch statement based on the server method parameter. So, in this method, we are only shutting down one type of server. I'm just suggesting that instead of this method calling cluster.stopProcessWithTimeout(tserver, 30, TimeUnit.SECONDS); it calls cluster.stopProcessWithTimeout(tabletServerProcesses, 30, TimeUnit.SECONDS); instead and inside of stopProcessWithTimeout we shut them all down in parallel.

Yea you're right. I was conflating this with stopping all server types at once. Within a single stop() call, order shouldnt matter. It looks like that is how it is in main already where stopProcessesWithTimeout() waits 30s per server type in total which makes more sense.

@DomGarguilo

Copy link
Copy Markdown
Member Author

In a7f6e98 I added a new method that accepts a list of processes, starts to shut them all down, then waits for them all under a single timer.

This should be in line with what you mentioned above @dlmarion

@dlmarion dlmarion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

@dlmarion

dlmarion commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Kicked off a full IT build

@dlmarion

dlmarion commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Full IT build successful, looks like it shaved about 20 minutes off the time. Great job!

@DomGarguilo
DomGarguilo merged commit e63a13a into apache:2.1 Oct 8, 2026
9 checks passed
@DomGarguilo
DomGarguilo deleted the ParallelstopProcs branch October 8, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants