Skip to content

RATIS-1603. TimeoutScheduler can have a huge amount of threads and cause OOM.#666

Merged
szetszwo merged 3 commits intoapache:masterfrom
szetszwo:RATIS-1603
Aug 6, 2022
Merged

RATIS-1603. TimeoutScheduler can have a huge amount of threads and cause OOM.#666
szetszwo merged 3 commits intoapache:masterfrom
szetszwo:RATIS-1603

Conversation

@szetszwo
Copy link
Contributor

@szetszwo szetszwo requested a review from codings-dan July 21, 2022 00:07
@szetszwo
Copy link
Contributor Author

@codings-dan , could you review this? It seems the new TimeoutTimer is safer than the existing TimeoutScheduler.

@codings-dan
Copy link
Contributor

@szetszwo Of course. I am testing if this patch can solve the problem, and will review the code after testing.

Copy link
Contributor

@codings-dan codings-dan left a comment

Choose a reason for hiding this comment

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

@szetszwo Thanks for working on this. I tested and found that OOM has been resolved. Overall LGTM, just left two comments for reference.

}

/** @return the number of scheduled but not completed timeout tasks. */
int getQueueSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we rename it to getTaskCount or a more explicit method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

import java.util.function.Supplier;

public final class TimeoutScheduler implements Closeable {
public final class TimeoutScheduler implements TimeoutExecutor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this class, it will cause thread overflow and should not be called again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep it for a while. Not sure if we may find some problem in the new TimeoutTimer in the future. It may be useful then.

Copy link
Contributor

@codings-dan codings-dan left a comment

Choose a reason for hiding this comment

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

The change looks good, +1

@szetszwo szetszwo merged commit 34867f2 into apache:master Aug 6, 2022
JoeCqupt pushed a commit to JoeCqupt/ratis that referenced this pull request Aug 7, 2022
codings-dan pushed a commit that referenced this pull request Aug 16, 2022
symious pushed a commit to symious/ratis that referenced this pull request Mar 6, 2024
@szetszwo szetszwo deleted the RATIS-1603 branch May 26, 2025 00:19
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.

2 participants