showuon commented on code in PR #17524:
URL: https://github.com/apache/kafka/pull/17524#discussion_r1857995583
##########
clients/src/main/java/org/apache/kafka/common/requests/DescribeClusterRequest.java:
##########
@@ -51,6 +52,13 @@ public String toString() {
public DescribeClusterRequest(DescribeClusterRequestData data, short
version) {
super(ApiKeys.DESCRIBE_CLUSTER, version);
this.data = data;
+ validateVersionForIncludingFencedBrokers(version,
data.includeFencedBrokers());
+ }
+
+ private void validateVersionForIncludingFencedBrokers(short version,
boolean includeFencedBrokers) {
+ if (version < 2 && includeFencedBrokers) {
+ throw new UnsupportedVersionException("Including fenced broker
endpoints is not supported with version " + version);
+ }
Review Comment:
1. Currently, the
`testDescribeClusterHandleUnsupportedVersionForIncludingFencedBrokers` will
return `UnsupportedVersionException` with `Api DESCRIBE_CLUSTER with version 1`
from
[here](https://github.com/apache/kafka/blob/48d60efe9802786451458aec9136b7b585fecd92/clients/src/test/java/org/apache/kafka/clients/MockClient.java#L261).
That's why it will fail. So, basically, it will fail with
`UnsupportedVersionException` as what we expected.
2. I understand why we need this different message as described in this
[comment](https://github.com/apache/kafka/pull/17524#discussion_r1850837344).
> We fallback to metadata request, if the broker does not support the new
DescribeCluster API. In this case, DescribeCluster API is supported but the
option to include fenced broker is not. So this is why we are checking the
specific error message to differentiate the cause of UnsupportedVersion
exception.
But I think using the error message as a condition is really weak. I think,
like you said, the reason we can't fallback to metadata request is because the
user added an option to _include fenced broker_. That is, when user requests to
"include fenced broker", this is the case that we should not fallback to
metadata request, even if it returned `UnsupportedVersionException`. After all,
the metadata request response won't include fenced broker, it is meaningless to
users. So, like how we treat `usingBootstrapControllers`, we can do similar
thing to this case, WDYT?
That is:
```
boolean handleUnsupportedVersionException(final UnsupportedVersionException
exception) {
if (metadataManager.usingBootstrapControllers()) {
return false;
}
// add some comments here
if (options.includeFencedBrokers()) {
return false;
}
```
Also, we should have an integration test for this unsupported version case.
Like [this
test](https://github.com/apache/kafka/blob/48d60efe9802786451458aec9136b7b585fecd92/core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala#L1354)
did.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
