lhotari edited a comment on pull request #9764: URL: https://github.com/apache/pulsar/pull/9764#issuecomment-789562722
> This map is used in many places throughout the code base and, depending on the use cases, there can be instances with a lot of entries or many instances with few entries. Also, the rate of this scanning through the map could be quite high in some cases. That's the reason why I have suggest that we evaluate each usage of `ConcurrentOpenHashMap.forEach` call and decide on a case-by-case basis how to fix the handling. > In light of that, I'm not convinced that a model where we always defensively copy the entire map when we need to do a map scan is a good general option, or a good replacement in the current code where most places assumed the scanning not be expensive (allocations-wise), even though there might be few specific cases in which it's appropriate. It's not really a defensive copy of the entire map. The copy is done one section at a time. Memory allocations and GC is extremely efficient in modern JVMs. Since it's about 28 bytes per entry, there would have to have millions of entries for the memory allocation overhead to have any noticeable significance. If necessary, this could be benchmarked. Since there aren't millions of entries, but usually a few thousand of entries, it has even less significance. This is why I don't see a reason to avoid the copying in this case. > If we do have blocking in that code that process the map items, and that cannot be easily removed, We do have IO operations (interactions with Zookeeper) in most of the code that processes the map items and it cannot be easily removed. > I believe a better alternative would be to make the scanning more flexible. > ... > Grab the stamp once per section and upgrade to read-lock only if there are write occurring The `.forEach` method in master branch [is already implemented this way "Grab the stamp once per section and upgrade to read-lock only if there are write occurring"](https://github.com/apache/pulsar/blob/110820fcf2e28e992b6263cf7c9c9d14aa67f941/pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenHashMap.java#L356-L395). It reduces the duration of the locks in the case that there are no writes. When there are writes, the `.forEach` processing will hold the read lock until all items have been processed. This is one of the reasons why the problems are rare with small number of items or low amount of write operations. However it doesn't help if the problems are rare, if the model is such that it could lead to a deadlock that isn't detected or resolved after some timeout. Currently the Pulsar broker locks up and doesn't resume operations if there's a deadlock. That's what makes the problem severe. Since the locks could be in specific sections of a ConcurrenOpenHashMap, it leads to partial unavailability which is hard to detect. Improving Pulsar stability is high priority and this should be addressed. Please don't take me wrong that I would think that this PR resolves all such issues. I know that it will only resolve the issues that are caused by a dead lock which could be avoided by not locking at all when processing items of a ConcurrentOpenHashMap. It would be interesting if @Vanlightly could take a look from a formal verification / modelling perspective on what would be practical in this case. @merlimat WDYT? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
