-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
EUREKA-1120 Move write.lock higher in stack... #1126
Conversation
…tion is performed while locked
Fixes #1120 |
Maybe I'm interpreting this wrong, but are the locks inverted? Could that be the root cause of this issue? Mutative(write) actions are acquiring a read lock eureka/eureka-core/src/main/java/com/netflix/eureka/registry/AbstractInstanceRegistry.java Lines 191 to 193 in ec2649e
eureka/eureka-core/src/main/java/com/netflix/eureka/registry/AbstractInstanceRegistry.java Lines 455 to 459 in ec2649e
Read operations are acquiring a write lock eureka/eureka-core/src/main/java/com/netflix/eureka/registry/AbstractInstanceRegistry.java Lines 865 to 871 in ec2649e
|
I don't think so, @mattnelson . RRWL's read lock can be entered by multiple threads, but if write lock is not locked, then other threads cannot enter even read lock. I think the intention here was to allow many threads modify different registered instances (statuses, metadata), but if response cache needs to be loaded - we take a pause and prepare response. But let's better wait for @qiangdavidliu to respond to your concern. |
@mgtriffid thanks for the PR. I checked some statistics and the data serialization (in extreme p99.9+ cases) can be pretty heavy, beyond even a second. With a write lock around this, this means instance registration and updates will be unable to proceed for that duration, which can be problematic. Seems like the root issue here is the mutable InstanceInfo objects to be serialized. It might be worth doing a quick experiment to see which is cheaper: locking around the serialization or ensuring copies of the data is returned by the |
Thanks for pointing this out, I actually was concerned about performance too, for some reason thought serialization is quick. Good that you have some stats from production of your scale.
Will update PR. |
…erialization is performed while locked" This reverts commit b0b011d
…to apps/delta request
Moved locks to original place, added copying constructors. |
Looks good, thanks @mgtriffid . Just want to verify with you that doing this is also able to solve your original issue? |
@qiangdavidliu , well, at least with this fix I cannot reproduce that issue, while without this fix I can do that easily. |
@qiangdavidliu Well, in our case, we consider frequent full registry fetches to be an issue. Especially in times when multiple clients execute full registry fetches at the same time, we have seen that we end up with resource starvation problems. What is your opinion on that? |
@pparth we run with a readonly tier internally (same REST API -> cache -> EurekaClient ---> write servers), which due to its nature mitigated the original issue referenced in this PR. |
The PR looks good, thank you @mgtriffid for the fix. |
so that payload serialization is performed while locked