frankvicky commented on code in PR #22405:
URL: https://github.com/apache/kafka/pull/22405#discussion_r3645858086
##########
clients/src/main/java/org/apache/kafka/common/errors/UnsupportedProtocolFieldException.java:
##########
@@ -16,6 +16,15 @@
*/
package org.apache.kafka.common.errors;
+import org.apache.kafka.common.annotation.InterfaceAudience;
+
+/**
+ * Indicates that a request contains a field or field value that is not
supported by the API version
+ * negotiated with the broker, and that a higher version would be required to
use it. This is a more
+ * specific subtype of {@link UnsupportedVersionException} that lets callers
distinguish an unsupported
+ * field from a wholly unsupported API version.
+ */
[email protected]
Review Comment:
Marking this class `@InterfaceAudience.Public` is a bigger change than it
looks — it turns the new exception into a public API surface, which in the JIRA
discussion was flagged as needing a KIP.
If we want it public we should file a small KIP; otherwise, could we drop
the annotation and keep it internal ?
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/InternalTopicManager.java:
##########
@@ -578,11 +578,7 @@ private Set<String> createTopics(final Set<NewTopic>
topicsToCreate,
if (cause instanceof UnsupportedProtocolFieldException) {
// An older broker rejected a field we rely on (e.g.
the default
// replication.factor=-1, which requires CreateTopics
request version 4+).
- throw new StreamsException(String.format(
- "Could not create topic %s, because brokers
don't support configuration replication.factor=-1."
- + " You can change the
replication.factor config or upgrade your brokers to version 2.4 or newer to
avoid this error.",
- topicName)
- );
+ throw new StreamsException(cause.getMessage());
Review Comment:
Two things I'd like to reconsider here:
1. The old message gave Streams users actionable guidance ("change the
replication.factor config or upgrade your brokers to version 2.4 or newer").
The new message ("Upgrade the cluster to CREATE_TOPICS API version >= 4 …") is
protocol-level and doesn't tell a Streams user what to do. Could we keep the
Streams-specific guidance and just append the cause's message (or use it as the
cause)?
Something like `new StreamsException("Could not create topic " + topicName
+ ": brokers don't support replication.factor=-1. Change replication.factor or
upgrade brokers to 2.4+.", cause)` — best of both worlds.
2. The new form drops `cause` from the `StreamsException`, so the stack
trace chain is lost. Please pass `cause` as the second arg.
--
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]
