[GitHub] [kafka] cmccabe commented on pull request #13280: KAFKA-14735: Improve KRaft metadata image change performance at high …

["cmccabe (via GitHub)" <gi...@apache.org>]

cmccabe commented on PR #13280:
URL: https://github.com/apache/kafka/pull/13280#issuecomment-1499641310

   Thanks for working on this, @rondagostino and @showuon ! I think this will be a really cool improvement.
   
   A note about namespace names:
   
   Rather than having an abbreviated namespace like `org.apache.kafka.pcoll` , how about `org.apache.kafka.server.persistent` ? This emphasizes that the code is in `server-common` and is more consistent with our other namespace names, which mostly don't use abbreviations.
   
   I don't have any objection to wrapping pcollections so that you can swap in a different library in the future. However, I don't like the wrappers here for two main reasons:
   1. They don't implement standard collection interfaces (java.util.Map, java.util.Set)
   2. They involve an additional "wrapper object" for every operation (every addition, etc.)
   
   The first issue is easy to fix. If you look at TimelineHashMap.java and TimelineHashSet.java, you can see that I implemented the standard Map and Set operations without too much fuss. It is a lot better than just implementing `Iterable` and some getters or having awkward "convert this to a real java collection" methods.
   
   The second issue is harder to fix because of the nature of Java's type system. However, I suspect that you might be able to do it by having something like a `PCollectionFactory` that implemented `PersistentCollectionFactory`. Then you could have methods like:
   
   ```
   class PersistentCollectionFactory<K, V> {
   Map<K, V> emptyMap<K, V>()
   Map<K, V> singletonMap<K, V>(K k, V v)
   Map<K, V> afterAdding<K, V>(Map<K, V> map, K k, V v)
   Map<K, V> afterRemoving<K, V>(Map<K, V> map, K k, V v)
   Map<K, V> afterRemoving<K, V>(Map<K, V> map, K k)
   ...
   }
   ```
   
   This would basically let you not have to cart around a wrapper object for each new map you created. However you could avoid dealing with pcollections types entirely in the calling code, and just refer to java.util.Map, etc. Since the pcollections type do implement the standard java types, this would work well here.


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org