MINOR: Cleanup Connect Module (1/n) #19869
Conversation
connect/api/src/main/java/org/apache/kafka/connect/data/Values.java
Outdated
Show resolved
Hide resolved
connect/api/src/test/java/org/apache/kafka/connect/data/ConnectSchemaTest.java
Show resolved
Hide resolved
| assertSchemaMatches(SchemaBuilder.map(Schema.STRING_SCHEMA, Schema.INT32_SCHEMA), new HashMap<String, Integer>()); | ||
| assertSchemaMatches(SchemaBuilder.map(Schema.STRING_SCHEMA, Schema.INT32_SCHEMA), Collections.singletonMap("a", 0)); | ||
| assertSchemaMatches(SchemaBuilder.map(Schema.STRING_SCHEMA, Schema.INT32_SCHEMA), Map.of("a", 0)); |
There was a problem hiding this comment.
It was leading to NPEs. The elements in the List may be null. Hence it must be kept as Arrays.asList.
|
Hey @m1a2st , Thanks for the review. |
|
Thanks for the review @m1a2st |
|
@sjhajharia could you please fix the conflicts? |
|
Hey @chia7712 |
There was a problem hiding this comment.
@sjhajharia thanks for this cleanup. only one minor comment remains.
| @@ -185,7 +184,7 @@ public SchemaBuilder doc(String doc) { | |||
|
|
|||
| @Override | |||
| public Map<String, String> parameters() { | |||
| return parameters == null ? null : Collections.unmodifiableMap(parameters); | |||
| return parameters == null ? null : Map.copyOf(parameters); | |||
There was a problem hiding this comment.
According to comment in line#198, the order is kept as expected. Perhaps, we should keep using Collections.unmodifiableMap
There was a problem hiding this comment.
Thank you @chia7712.
I have updated the PR.
| @@ -419,7 +419,7 @@ public Schema valueSchema() { | |||
| */ | |||
| public Schema build() { | |||
| return new ConnectSchema(type, isOptional(), defaultValue, name, version, doc, | |||
| parameters == null ? null : Collections.unmodifiableMap(parameters), | |||
| parameters == null ? null : Map.copyOf(parameters), | |||
There was a problem hiding this comment.
This will change the order too, right?
There was a problem hiding this comment.
Ah yes.
Missed that.
Fixing the same in the next commit
|
Hey @chia7712 |
There was a problem hiding this comment.
@sjhajharia thanks for this patch. one last comment is left. PTAL
| @@ -1273,10 +1254,8 @@ public boolean canDetect(Object value) { | |||
| } | |||
| if (knownType == null) { | |||
| knownType = schema.type(); | |||
| } else if (knownType != schema.type()) { | |||
There was a problem hiding this comment.
if (knownType == null) knownType = schema.type();
return knownType == schema.type();How about using this style?
There was a problem hiding this comment.
Makes sense. I will push an update.
Thanks!
Now that Kafka support Java 17, this PR makes some changes in connect
module. The changes in this PR are limited to only some files. A future
PR(s) shall follow.
The changes mostly include:
Arrays.asList() are replaced with List.of()
with Map.of()
Sub modules targeted: api, basic-auth-extensions, file, json, mirror,
mirror-client