You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mjsax (via GitHub)" <gi...@apache.org> on 2025/02/08 05:21:50 UTC
[PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
mjsax opened a new pull request, #18840:
URL: https://github.com/apache/kafka/pull/18840
(no comment)
--
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
Re: [PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #18840:
URL: https://github.com/apache/kafka/pull/18840#discussion_r1947484523
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -234,6 +281,7 @@ public void testGetRestoreConsumerConfigs() {
assertNull(returnedProps.get(ConsumerConfig.GROUP_ID_CONFIG));
}
+ @SuppressWarnings("resource")
Review Comment:
Some side cleanup to avoid IntelliJ warning. More if it below.
--
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
Re: [PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on PR #18840:
URL: https://github.com/apache/kafka/pull/18840#issuecomment-2649101289
Java 17:
```
Found 2 flaky test failures:
FLAKY ⚠️ CooperativeConsumerCoordinatorTest > testOutdatedCoordinatorAssignment()
FLAKY ⚠️ KRaftClusterTest > testDescribeQuorumRequestToControllers()
```
Java 23:
```
Found 3 flaky test failures:
FLAKY ⚠️ KRaftClusterTest > testDescribeQuorumRequestToControllers()
FLAKY ⚠️ ReplicationQuotasTest > initializationError
FLAKY ⚠️ RequestQuotaTest > initializationError
```
--
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
Re: [PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #18840:
URL: https://github.com/apache/kafka/pull/18840#discussion_r1947484456
##########
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java:
##########
@@ -682,7 +682,7 @@ public class StreamsConfig extends AbstractConfig {
/** {@code processor.wrapper.class} */
public static final String PROCESSOR_WRAPPER_CLASS_CONFIG = "processor.wrapper.class";
- public static final String PROCESSOR_WRAPPER_CLASS_DOC = "A processor wrapper class or class name that implements the <code>org.apache.kafka.streams.state.ProcessorWrapper</code> interface. "
+ static final String PROCESSOR_WRAPPER_CLASS_DOC = "A processor wrapper class or class name that implements the <code>org.apache.kafka.streams.state.ProcessorWrapper</code> interface. "
Review Comment:
This is the root cause for the PR. Saw this by chance...
KIP-1112 correctly declares it as `private` -- we need to make it package-private though.,to allow `TopologyConfig` to use it. It should not be public.
As it seems we keep leaking these members (and it could happen again in the future), I added a test for it below (to avoid that we need to do another KIP to remove it again in the future... cf https://cwiki.apache.org/confluence/display/KAFKA/KIP-1085%3A+Fix+leaking+*_DOC+variables+in+StreamsConfig)
Took the liberty to fix this in 4.0 directly: https://github.com/apache/kafka/commit/562ecf4a83c2fa3577c45eba02328a1bfb751c2a
--
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
Re: [PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax merged PR #18840:
URL: https://github.com/apache/kafka/pull/18840
--
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
Re: [PR] MINOR: verify that internal StreamsConfig members are not public [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #18840:
URL: https://github.com/apache/kafka/pull/18840#discussion_r1947484637
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -1002,9 +1052,9 @@ public void shouldGetClientTagsMapWhenSet() {
props.put(StreamsConfig.clientTagPrefix("cluster"), "cluster-1");
final StreamsConfig config = new StreamsConfig(props);
final Map<String, String> clientTags = config.getClientTags();
- assertEquals(clientTags.size(), 2);
- assertEquals(clientTags.get("zone"), "eu-central-1a");
- assertEquals(clientTags.get("cluster"), "cluster-1");
+ assertEquals(2, clientTags.size());
Review Comment:
Correct order is `assertEquals(expectedValue, actualValue)`
--
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