You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "ijuma (via GitHub)" <gi...@apache.org> on 2023/09/22 13:26:55 UTC
[GitHub] [kafka] ijuma opened a new pull request, #14427: MINOR: Fix "this-escape" compiler warnings introduced by JDK 21
ijuma opened a new pull request, #14427:
URL: https://github.com/apache/kafka/pull/14427
This is one of the steps required for kafka to compile with Java 21.
The fixes are one of two types:
1. Suppress warning if fixing would potentially result in an incompatible change (for public classes)
2. Add final to one or more methods so that the escape is not possible
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334734260
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
That is the right guide for features that are GA. I am not sure if we have updated the javadoc configuration for tiered storage yet.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334524883
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java:
##########
@@ -60,6 +60,7 @@ public TimeIndex(File file, long baseOffset, int maxIndexSize) throws IOExceptio
this(file, baseOffset, maxIndexSize, true);
}
+ @SuppressWarnings("this-escape")
Review Comment:
I think this might be due to super class methods such as maxEntries(). We can make them final. AbstractIndex is not a public class [1]
(same for other index files)
[1] https://kafka.apache.org/36/javadoc/allclasses-index.html
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
the warning occurs probably because we call `newApiMessage` which is protected and might be overridden in a sub-class? If that is the case, could we make it final?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java:
##########
@@ -343,7 +343,7 @@ public LogConfig(Map<?, ?> props) {
this(props, Collections.emptySet());
}
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"deprecation", "this-escape"})
Review Comment:
self note: this warning occurs due to methods such as getBoolean() from AbstractConfig, which is a public class and we can't add `final` to those methods.
##########
storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java:
##########
@@ -56,6 +56,7 @@ public class LeaderEpochFileCache {
* @param topicPartition the associated topic partition
* @param checkpoint the checkpoint file
*/
+ @SuppressWarnings("this-escape")
public LeaderEpochFileCache(TopicPartition topicPartition, LeaderEpochCheckpoint checkpoint) {
Review Comment:
This is probably due to calling `this.assign()` which is a private method. The code works because it is the last line in the ctor but it's a legitimate warning. Should we create a ticket for fixing this correctly (and removing the warning)?
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334585052
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
I thought this was in a public package. @satishd can tell us if it's ok to make it private.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334428308
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConnector.java:
##########
@@ -381,15 +381,15 @@ private void doTransitionTo(TargetState targetState) throws Throwable {
}
}
- public boolean isSinkConnector() {
+ public final boolean isSinkConnector() {
Review Comment:
@C0urante Is this OK or are these classes public and subclassable? Similar for other changes affecting connect runtime classes.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334642477
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
I usually use https://kafka.apache.org/36/javadoc/allclasses-index.html as a guide on what is public and what isn't. If this class in public, we are probably missing it from here. From what I know, this class is used only by TopicBased RLMM which is our internal implementation of RLMM plugin (hence not public).
Let's wait to hear from Satish.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334438340
##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -227,8 +226,9 @@ private <T> T createDelegate(ClassLoader classLoader, String className) {
}
}
+ @SuppressWarnings("removal")
Review Comment:
These are existing deprecation warnings - they're not failing the build because it's a Java class in the `core` module and our compilation configs for Java in this mixed Scala/Java module don't seem to be consistent with the rest.
--
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
[GitHub] [kafka] ijuma commented on pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14427:
URL: https://github.com/apache/kafka/pull/14427#issuecomment-1731708017
> Please add to the description - "this-escape" is related to new lint warning
Done.
--
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
[GitHub] [kafka] satishd commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334967466
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
It is not a public API. We should be able to make any required changes to this class without worrying about API compatibility.
We plan to build a tool to inspect messages in the remote log metadata topic. That tool may use some of these classes in future.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334649199
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java:
##########
@@ -60,6 +60,7 @@ public TimeIndex(File file, long baseOffset, int maxIndexSize) throws IOExceptio
this(file, baseOffset, maxIndexSize, true);
}
+ @SuppressWarnings("this-escape")
Review Comment:
Hmmm, just for my curiosity, the super class object is guaranteed to be created in the sub-class ctor because `super()` is the first line in sub-class ctor. Hence, there is no risk (of a partial object) if we call it's non-overloadable methods because the super class instance whose methods we are referring to will be completely constructed. Then why is this a warning?
(nevermind if you don't have the answer on top of your head. Java language semantics is not my strong suite so I might be missing something here)
--
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
[GitHub] [kafka] ijuma commented on pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14427:
URL: https://github.com/apache/kafka/pull/14427#issuecomment-1732392148
JDK 20 build passed, the others failed due to flaky tests.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334596556
##########
storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java:
##########
@@ -56,6 +56,7 @@ public class LeaderEpochFileCache {
* @param topicPartition the associated topic partition
* @param checkpoint the checkpoint file
*/
+ @SuppressWarnings("this-escape")
public LeaderEpochFileCache(TopicPartition topicPartition, LeaderEpochCheckpoint checkpoint) {
Review Comment:
The problem is that we pass `this` to `forEach`. Not sure there's a lot of value in fixing this.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1335061842
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConnector.java:
##########
@@ -381,15 +381,15 @@ private void doTransitionTo(TargetState targetState) throws Throwable {
}
}
- public boolean isSinkConnector() {
+ public final boolean isSinkConnector() {
Review Comment:
Thanks @C0urante!
--
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
[GitHub] [kafka] ijuma commented on pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #14427:
URL: https://github.com/apache/kafka/pull/14427#issuecomment-1732423752
JDK 8 build passed, the rest had unrelated flaky failures:
> Build / JDK 11 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft
> Build / JDK 11 and Scala 2.13 / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
> Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.processor.internals.assignment.StickyTaskAssignorTest.shouldAssignRandomInput[enableRackAwareTaskAssignor=true]
> Build / JDK 20 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testDelayedConfigurationOperations()
> Build / JDK 20 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[6] Type=Raft-Isolated, Name=testDescribeQuorumStatusSuccessful, MetadataVersion=3.7-IV0, Security=PLAINTEXT
> Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector
> Build / JDK 17 and Scala 2.13 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testSendOffsetsWithGroupMetadata(String).quorum=kraft
> Build / JDK 17 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[1] Type=Raft-Combined, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.7-IV0, Security=PLAINTEXT
> Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334584079
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java:
##########
@@ -60,6 +60,7 @@ public TimeIndex(File file, long baseOffset, int maxIndexSize) throws IOExceptio
this(file, baseOffset, maxIndexSize, true);
}
+ @SuppressWarnings("this-escape")
Review Comment:
No, that doesn't work. It only works if the method is in the same class. It's already escaped if it's a superclass.
--
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
[GitHub] [kafka] C0urante commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1334606614
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConnector.java:
##########
@@ -381,15 +381,15 @@ private void doTransitionTo(TargetState targetState) throws Throwable {
}
}
- public boolean isSinkConnector() {
+ public final boolean isSinkConnector() {
Review Comment:
This is fine; none of the Connect- or MM2-related files touched in the PR are part of public API.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1335061735
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java:
##########
@@ -60,6 +60,7 @@ public TimeIndex(File file, long baseOffset, int maxIndexSize) throws IOExceptio
this(file, baseOffset, maxIndexSize, true);
}
+ @SuppressWarnings("this-escape")
Review Comment:
I think the current implementation is conservative when it comes to any assumptions for separate compilation units. After all, one can change the implementation of the superclass method to invoke a method that is overridden/implemented by the subclass without recompiling the subclass. I did test various variations and it doesn't seem possible to fix the warning while invoking any superclass methods.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1335061735
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java:
##########
@@ -60,6 +60,7 @@ public TimeIndex(File file, long baseOffset, int maxIndexSize) throws IOExceptio
this(file, baseOffset, maxIndexSize, true);
}
+ @SuppressWarnings("this-escape")
Review Comment:
I think the current implementation is conservative when it comes to any assumptions for separate compilation units. After all, one can change the implementation of the superclass method to invoke a method that is overridden/implemented by the subclass without recompiling the subclass. I did test various variations and it doesn't seem possible to fix the warning while invoking any superclass methods from the constructor (directly or indirectly).
--
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
[GitHub] [kafka] ijuma merged pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma merged PR #14427:
URL: https://github.com/apache/kafka/pull/14427
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #14427: KAFKA-15485: Fix "this-escape" compiler warnings introduced by JDK 21 (1/N)
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14427:
URL: https://github.com/apache/kafka/pull/14427#discussion_r1335059135
##########
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##########
@@ -50,6 +50,7 @@ public class RemoteLogMetadataSerde {
private final Map<Short, RemoteLogMetadataTransform> keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+ @SuppressWarnings("this-escape")
Review Comment:
Thanks. I removed the suppression and made both protected methods final.
--
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