You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/03 16:25:50 UTC
[GitHub] [kafka] Fleshgrinder opened a new pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Fleshgrinder opened a new pull request #9369:
URL: https://github.com/apache/kafka/pull/9369
This allows the configuration values for CompressionType in the ProducerConfig and the OffsetResetStrategy in the ConsumerConfig to be specified regardless of their case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-711638037
I can try to find the others.
Why inconsistent? This is the classical robustness principle: be lean in what you accept and strict in what you produce.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#discussion_r507452731
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/OffsetResetStrategy.java
##########
@@ -16,6 +16,40 @@
*/
package org.apache.kafka.clients.consumer;
+import java.util.Locale;
+import java.util.Objects;
+
+/**
+ * @see ConsumerConfig#AUTO_OFFSET_RESET_CONFIG
+ * @see ConsumerConfig#AUTO_OFFSET_RESET_DOC
+ */
public enum OffsetResetStrategy {
- LATEST, EARLIEST, NONE
+ LATEST, EARLIEST, NONE;
+
+ // Enums are singletons, this means that this conversion happens once the
+ // first time a variant is actually used, and never again.
+ private final String id = name().toLowerCase(Locale.ROOT);
Review comment:
Sorry that I can't catch your point here. Could you give more explanation to me? Is it different to ```Enum#toString```?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-703280878
Failed tests seem unrelated to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-703703414
> The code change looks good to me. Should we have some tests for this?
Always I'd say. 😊
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#discussion_r508639180
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/OffsetResetStrategy.java
##########
@@ -16,6 +16,40 @@
*/
package org.apache.kafka.clients.consumer;
+import java.util.Locale;
+import java.util.Objects;
+
+/**
+ * @see ConsumerConfig#AUTO_OFFSET_RESET_CONFIG
+ * @see ConsumerConfig#AUTO_OFFSET_RESET_DOC
+ */
public enum OffsetResetStrategy {
- LATEST, EARLIEST, NONE
+ LATEST, EARLIEST, NONE;
+
+ // Enums are singletons, this means that this conversion happens once the
+ // first time a variant is actually used, and never again.
+ private final String id = name().toLowerCase(Locale.ROOT);
Review comment:
We can move the code to `toString` but than the `toLowerCase` conversion is executed every time `toString` is invoked. This way the conversion happens once: the first time the enum is used. From there on out the string stays and never needs to be recomputed. Meaning:
```java
System.out.println(OffsetResetStrategy.LATEST); // first access, `id` member is populated with `toLowerCase` result
System.out.println(OffsetResetStrategy.LATEST); // just uses `id`
System.out.println(OffsetResetStrategy.LATEST); // just uses `id`
System.out.println(OffsetResetStrategy.LATEST); // just uses `id`
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#discussion_r508641877
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java
##########
@@ -361,7 +363,7 @@ public static StreamThread create(final InternalTopologyBuilder builder,
final AtomicLong nextScheduledRebalanceMs = new AtomicLong(Long.MAX_VALUE);
consumerConfigs.put(StreamsConfig.InternalConfig.NEXT_SCHEDULED_REBALANCE_MS, nextScheduledRebalanceMs);
- final String originalReset = (String) consumerConfigs.get(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG);
+ final String originalReset = OffsetResetStrategy.forName((String) consumerConfigs.get(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG)).toString();
Review comment:
The value could be in any format: `LaTeSt`, `latest`, `LATEST`, … but we are interested only in its canonical form and the canonical form is provided through the Enum, hence, we have to parse it again. The alternative would be to call `toLowerCase(Locale.ROOT)` on it but we would effectively be duplicating the logic and that's very bad.
Ideal would actually be if our configuration is not giving us a string but rather the Enum member that was configured but I think we all know the our configuration is generally not ideal, so that's an issue for another time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-703701703
Thanks for the PR @Fleshgrinder ,
The code change looks good to me. Should we have some tests for this?
Thanks!
-John
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#discussion_r507454409
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java
##########
@@ -361,7 +363,7 @@ public static StreamThread create(final InternalTopologyBuilder builder,
final AtomicLong nextScheduledRebalanceMs = new AtomicLong(Long.MAX_VALUE);
consumerConfigs.put(StreamsConfig.InternalConfig.NEXT_SCHEDULED_REBALANCE_MS, nextScheduledRebalanceMs);
- final String originalReset = (String) consumerConfigs.get(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG);
+ final String originalReset = OffsetResetStrategy.forName((String) consumerConfigs.get(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG)).toString();
Review comment:
Not sure whether this change is necessary. The value should be validated already.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-711644549
> Why inconsistent? This is the classical robustness principle: be lean in what you accept and strict in what you produce.
sorry for unclear description. My point was that all enum-related config should be case-insensitive so users don't need to check the source code to avoid encountering case issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-711562724
@Fleshgrinder I like this idea of ignoring case for enum type. However, it would be better to change all enum-related configs at once (and it may need KIP) since users may be confused by this inconsistent checks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on pull request #9369: KAFKA-4715: Ignore case of CompressionType and OffsetResetStrategy
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9369:
URL: https://github.com/apache/kafka/pull/9369#issuecomment-704098722
@vvcephei 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org