-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][meta] Use ForkJoinPool.commonPool to handle Metadata operations #22542
[fix][meta] Use ForkJoinPool.commonPool to handle Metadata operations #22542
Conversation
I didn't review in detail yet. I think something like this has come up before. This might have an impact on correctness since ordering guarantees would be lost. |
b153708
to
8629e9d
Compare
8629e9d
to
d832553
Compare
return store.get(prefix + "/e1").thenApply((ignore2) -> { | ||
verify.run(); | ||
return null; | ||
}).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is a mix between sync & async calls.
eg: blocking in a callback thread.
Using ForkJoinPool, it's only moving the problem to a different place. It will be able to avoid deadlocking on a simple 1:1 situation, though when the ForkJoinPool threads count is maxed, we will still be subject to deadlock.
No matter how we handle this, it's going to be problematic, hence we always have had to be careful in avoiding blocking calls to metadata.
Questions:
|
Although ForkJoinPool can grow very large(max 32767, commonPool's maximumPoolSize is 256 by default) -- it is less likely exhausted (Should be more than the number of caller threads), I agree that we should not chain the "heavy operation" (in this case, metadata sync calls) after the metadata operation. I will fix the caller code not to trigger the metadata sync call, instead. |
Motivation
Observed that the metadata store executor thread can be blocked when handling chained future operations (e.g. while running handleGetResult). Especially, when the chained future operation needs to wait for another metadata access, this can block the metadata get/put operations, because the flush() will never be called(because the executor is blocked now).
Modifications
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: heesung-sn#65