mjsax commented on code in PR #17422:
URL: https://github.com/apache/kafka/pull/17422#discussion_r1794397939
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -1304,22 +1307,72 @@ public void
shouldDisableMetricCollectionForAllInternalClients() {
}
@Test
- public void shouldDisableMetricCollectionOnMainConsumerOnly() {
+ public void
shouldThrowConfigExceptionWhenMainConsumerMetricsDisabledStreamsMetricsPushEnabled()
{
+
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+
+ final Exception exception = assertThrows(ConfigException.class, () ->
new StreamsConfig(props));
+
+ assertThat(
+ exception.getMessage(),
+ containsString("KafkaStreams has metrics push enabled" +
+ " but the main consumer metrics push is disabled.
Enable " +
+ " metrics push for the main consumer")
+ );
+ }
+
+ @Test
+ public void
shouldThrowConfigExceptionConsumerMetricsDisabledStreamsMetricsPushEnabled() {
+
props.put(StreamsConfig.consumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+ final Exception exception = assertThrows(ConfigException.class, () ->
new StreamsConfig(props));
+
+ assertThat(
+ exception.getMessage(),
+ containsString("KafkaStreams has metrics push enabled" +
+ " but the consumer clients metrics push is disabled.
Enable " +
+ " metrics push for the main consumer")
+ );
+ }
+
+ @Test
+ public void
shouldEnableMetricsForMainConsumerWhenConsumerPrefixDisabledMetricsPushEnabled()
{
+
props.put(StreamsConfig.consumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
true);
Review Comment:
I think we should add one more test, with `consumer.xxx = true` and
`main.consumer.xxx = false` and expect a `ConfigException`.
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -1304,22 +1307,72 @@ public void
shouldDisableMetricCollectionForAllInternalClients() {
}
@Test
- public void shouldDisableMetricCollectionOnMainConsumerOnly() {
+ public void
shouldThrowConfigExceptionWhenMainConsumerMetricsDisabledStreamsMetricsPushEnabled()
{
+
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+
+ final Exception exception = assertThrows(ConfigException.class, () ->
new StreamsConfig(props));
+
+ assertThat(
+ exception.getMessage(),
+ containsString("KafkaStreams has metrics push enabled" +
+ " but the main consumer metrics push is disabled.
Enable " +
+ " metrics push for the main consumer")
+ );
+ }
+
+ @Test
+ public void
shouldThrowConfigExceptionConsumerMetricsDisabledStreamsMetricsPushEnabled() {
+
props.put(StreamsConfig.consumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+ final Exception exception = assertThrows(ConfigException.class, () ->
new StreamsConfig(props));
+
+ assertThat(
+ exception.getMessage(),
+ containsString("KafkaStreams has metrics push enabled" +
+ " but the consumer clients metrics push is disabled.
Enable " +
Review Comment:
Does it add much value to say `consumer clients` for this case instead of
`main consumer clients`? Wondering if we can simplify the verification by
printing the same error message for both cases. Given that `consumer.xxx` does
disable it for the main consumer, printing `main consumer` in the error message
would still be correct?
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -1304,22 +1307,72 @@ public void
shouldDisableMetricCollectionForAllInternalClients() {
}
@Test
- public void shouldDisableMetricCollectionOnMainConsumerOnly() {
+ public void
shouldThrowConfigExceptionWhenMainConsumerMetricsDisabledStreamsMetricsPushEnabled()
{
+
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
+
+ final Exception exception = assertThrows(ConfigException.class, () ->
new StreamsConfig(props));
+
+ assertThat(
+ exception.getMessage(),
+ containsString("KafkaStreams has metrics push enabled" +
+ " but the main consumer metrics push is disabled.
Enable " +
+ " metrics push for the main consumer")
+ );
+ }
+
+ @Test
+ public void
shouldThrowConfigExceptionConsumerMetricsDisabledStreamsMetricsPushEnabled() {
+
props.put(StreamsConfig.consumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
props.put(StreamsConfig.mainConsumerPrefix(ConsumerConfig.ENABLE_METRICS_PUSH_CONFIG),
false);
Review Comment:
Given that we test `consumer.xxx` it seem we should remove this line?
--
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]
