kevin-wu24 commented on code in PR #23128:
URL: https://github.com/apache/kafka/pull/23128#discussion_r4008332485
##########
metadata/src/main/java/org/apache/kafka/image/ScramImage.java:
##########
@@ -46,7 +46,10 @@ public record ScramImage(Map<ScramMechanism, Map<String,
ScramCredentialData>> m
public static final ScramImage EMPTY = new ScramImage(Map.of());
public ScramImage {
- mechanisms = Collections.unmodifiableMap(mechanisms);
+ Map<ScramMechanism, Map<String, ScramCredentialData>> copiedMechanisms
= new HashMap<>();
+ mechanisms.forEach((mechanism, credentials) ->
+ copiedMechanisms.put(mechanism, Collections.unmodifiableMap(new
HashMap<>(credentials))));
Review Comment:
It looks like there is a convention of wrapping maps in `unmodifiableMap` in
image record compact constructors. I suspect the motivation behind this is
`unmodifiableMap` is O(1), and can prevent certain "bad code" from modifying
the image at test/runtime. However, it is probably not performant to actually
deep copy all of the kafka objects contained in the various `...Image` classes.
The reason why I believe this is "okay" is below:
The call-sites of all of the metadata sub-images (SCRAM, topics, cluster,
etc.) compact constructors are all on the single-threaded `MetadataLoader`'s
event queue, which is the only thread the writing/construction of any given
`MetadataImage` can occur. There is no risk of concurrent access there. There
may be things that read a `MetadataImage`'s data which live on other threads
(e.g. request handling), but each `...Image` is a `record` to make its internal
data immutable. Code that reads the image and modifies any underlying image
data is obviously wrong, because the image represents on-disk metadata, that
can only be changed via KRaft replication.
This code is not doing a true deep copy of each `ScramCredentialData`
object. That is fine, because this method is O(n) where n is the size of the
larger map. This is much cheaper than linear with the number of
`ScramCredentialData` byte arrays probably. However, it makes the included
changes in `ScramCredentialData` redundant IMO.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
