kevin-wu24 commented on code in PR #23128:
URL: https://github.com/apache/kafka/pull/23128#discussion_r4027928588
##########
metadata/src/test/java/org/apache/kafka/image/ScramImageTest.java:
##########
@@ -67,6 +71,27 @@ public void testImage2RoundTrip() {
testToImage(IMAGE2);
}
+ @Test
+ public void testMechanismMapsAreImmutableSnapshots() {
+ ScramCredentialData credential = new ScramCredentialData(
+ new byte[] {1}, new byte[] {2}, new byte[] {3}, 4096);
+ Map<String, ScramCredentialData> credentials = new HashMap<>();
+ credentials.put("alice", credential);
+ Map<ScramMechanism, Map<String, ScramCredentialData>> mechanisms = new
HashMap<>();
+ mechanisms.put(ScramMechanism.SCRAM_SHA_256, credentials);
+
+ ScramImage image = new ScramImage(mechanisms);
+
+ mechanisms.clear();
+ credentials.clear();
+ assertEquals(
+ Map.of(ScramMechanism.SCRAM_SHA_256, Map.of("alice", credential)),
+ image.mechanisms());
Review Comment:
This passes because we copy the nested map's contents.
##########
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:
Your current implementation is copying over each inner-map's contents into a
new HashMap. For the same reasons as previously discussed, I don't think this
is necessary.
The metadata image publishing pipeline only has one image at a time. When
deltas are applied to the current image, which changes the contents, a new
image is constructed. Code that tries to modify `MetadataImage` contents
outside of `MetadataLoader/BatchLoader/Delta` is incorrect.
```suggestion
Map<ScramMechanism, Map<String, ScramCredentialData>>
copiedMechanisms = new HashMap<>(mechanisms.size());
mechanisms.forEach((mechanism, credentials) ->
copiedMechanisms.put(mechanism,
Collections.unmodifiableMap(credentials)));
```
--
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]
