vvcephei commented on pull request #8163: URL: https://github.com/apache/kafka/pull/8163#issuecomment-636153714
Just managed to dig deep enough into my review backlog to encounter this PR. Sorry, all, for the delay. It sounds like this whole idea is a performance improvement, so it seems dubious to consider merging it without any benchmarks at all. Personally, I'd like to see a JMH benchmark for this kind of thing before we bother with the fullly integrated benchmark suite. Another concern is the thread safety angle. After some brief reading on TreeMap vs ConcurrentSkipListMap, it seems like TreeMap isn't completely unsafe. If you have a single writer and multiple readers, TreeMap should be mostly ok, except that it can throw a ConcurrentModificationException if the map is modified by the writer while readers are iterating over it. To what extent are we willing to tolerate ConcurrentModificationExceptions in IQ? I'm also a little unclear on whether we need a memory barrier or not. It seems like this ticket is more about researching the issues and benchmarking than just mechanically swapping one implementation for the other. ---------------------------------------------------------------- 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: us...@infra.apache.org
