[GitHub] [kafka] cmccabe commented on pull request #10949: KAFKA-13019: Add MetadataImage and MetadataDelta classes for KRaft Snapshots

[GitBox <gi...@apache.org>]

cmccabe commented on pull request #10949:
URL: https://github.com/apache/kafka/pull/10949#issuecomment-871756196


   > For the Image classes where the underlying cache is just a Map, it seems unsafe to expose that map through the accessor. Should we make a defensive copy? Or perhaps we could have some other methods for accessing the map without allowing mutation, e.g.
   > 
   > ```java
   > public final class ClusterImage {
   >     // ...
   >     public void brokers(BiConsumer<Integer, BrokerRegistration> brokerConsumer) {
   >         // ...
   >     }
   > }
   > ```
   
   I did try to hide the maps, initially, but it just gets too cumbersome to try to do everything through accessors.
   
   Copying the map would be a huge performance hit. Something like using `Collections#unmodifiableMap` is probably OK. Although it still adds some overhead, it's probably not too bad... I will put that in place for now, at least until performance testing tells us otherwise.


-- 
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