cmccabe commented on PR #13280: URL: https://github.com/apache/kafka/pull/13280#issuecomment-1500464493
> It is true that the wrapper implementation in the PR as it stands right now always creates a new object when creating a new persistent collection (either by creating an empty/singleton or by adding/removing from an existing one). The overhead is low -- object creation isn't onerous in time or space when object initialization is cheap (which it is here), and the work created by having the additional object will be a very low percentage of the work that the underlying persistent collection has to do anyway. So it doesn't concern me very much in that regard, but I do agree we should avoid it if we decide it is unnecessary and any tradeoff ends up being worth it. The performance issues are debatable, I'll definitely grant you that. And I feel kind of bad mentioning them considering what a big performance improvement this will be in general. Probably the bigger issue is the extra complexity of implementing our own Set / Map / TreeMap / etc. It's all doable (and perhaps we'll end up doing it for Vavr) but it feels kind of bad when the library has already done that work. > So I think we have a situation where the current PR introduces a small amount of overhead that is not expected to be significant but that we would prefer to eliminate of possible. But the cost of eliminating it is a lack of type safety -- the compiler would not be able to know if a collection is a persistent one or not. I agree that the solution I proprosed lacks type safety. It would be possible for someone to pass a non-persistent map to one of the `PersistentCollectionFactory` methods. Or, of course, to pass a different kind of persistent collection than what is expected. If type safety is a requirement, it would be possible to add a third generic parameter which is the type of the Map. But this gets back to your original objection to exposing these types, which is that people could then invoke methods directly on the pcollections map type. The users would also need to pull in the pcollections imports and so on. I feel that there is no advantage to going down this path versus just using pcollections directly, since the effort to switch to something else later would not be lessened. Another option is to actually handle the different types in an appropriate fashion. For example, if someone gives you a non-pcollections `java.util.HashMap`, you can implement `afterAdding` by copying it with the requested addition. Hmm... now that I think about this more, it actually sounds like a pretty good option. I suppose in that case we could just have a bunch of static methods in `org.apache.kafka.server.persistent.PersistentCollection` like `PersistentCollection. afterAdding(K k, V v)` and implement them appropriately for all persistent collection types we support... -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
