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 "bad code" from modifying the image
at test/runtime. 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.
##########
metadata/src/main/java/org/apache/kafka/metadata/ScramCredentialData.java:
##########
@@ -40,21 +46,36 @@ public static ScramCredentialData fromRecord(
record.iterations());
}
+ @Override
+ public byte[] salt() {
+ return Arrays.copyOf(salt, salt.length);
+ }
+
+ @Override
+ public byte[] storedKey() {
+ return Arrays.copyOf(storedKey, storedKey.length);
+ }
+
+ @Override
+ public byte[] serverKey() {
+ return Arrays.copyOf(serverKey, serverKey.length);
+ }
+
public UserScramCredentialRecord toRecord(
String userName,
ScramMechanism mechanism
) {
return new UserScramCredentialRecord().
setName(userName).
setMechanism(mechanism.type()).
- setSalt(salt).
- setStoredKey(storedKey).
- setServerKey(serverKey).
+ setSalt(salt()).
+ setStoredKey(storedKey()).
+ setServerKey(serverKey()).
setIterations(iterations);
}
public ScramCredential toCredential() {
- return new ScramCredential(salt, storedKey, serverKey, iterations);
+ return new ScramCredential(salt(), storedKey(), serverKey(),
iterations);
Review Comment:
Both of these methods, which are the only callers of the newly introduced
deep-copy getters, are only called on the single-threaded image publishing
pipeline. IMO, they can be removed.
This object is a in-memory representation of a binary metadata record on
disk. Any modification of this object by the code downstream of
`toCredential()` does not make sense.
--
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]
