[
https://issues.apache.org/jira/browse/KAFKA-15768?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17790603#comment-17790603
]
John Roesler commented on KAFKA-15768:
--------------------------------------
Hey [~hanyuzheng] , thanks for the bug report!
I agree with you that if there is exactly one partition responding and it
responds with a FailedQueryResult, then it could make sense to return it
instead of throwing an exception.
However, I do want to clarify that an expected usage of this method is to do
something like issue a key-based query to multiple partitions (because you
don't know which partition hosts the key, if any) and then getting the result
back from whichever partition responded with a non-null result. In other words,
it should return the result if and only if all queried partitions responded
successfully AND at most one partition returned a non-null result.
>From that perspective, it might actually be *more confusing* to return the
>FailedQueryResult instead of throwing an exception in the
>single-partition-response case, since it means that callers have two error
>paths to handle. I.e., they will have to write code like:
```
{{try {}}
{{ }}{{onlyResult = result.getOnlyPartitionResult()}}
{{ if (onlyResult.isSuccessful()) {}}
{{ doSomething(onlyResult);}}
{{ } else {}}
{{ handleFailureFromResult(onlyResult);}}
{{ }}}
{{} catch (RuntimeException e) {}}
{{ handleFailureFromException(e);}}
{{}}}
```
Answer to the side-note question: Maybe it's a bit philosophical, but the
object upon which you call an instance method is in some sense an argument to
the method (e.g., `this` is always an argument to an instance method). I don't
think IllegalStateException would be better, since the application isn't in an
illegal state (an illegal state is one which the code author thinks should
never be reached, I.e., it would indicate a programming error within the
framework).
I could see an "argument" for choosing another exception type, or maybe
declaring one for this purpose. I.e., something like an "assertion violated"
exception might be more clear than IllegalArgumentException.
> StateQueryResult#getOnlyPartitionResult should not throw for FailedQueryResult
> ------------------------------------------------------------------------------
>
> Key: KAFKA-15768
> URL: https://issues.apache.org/jira/browse/KAFKA-15768
> Project: Kafka
> Issue Type: Bug
> Components: streams
> Reporter: Matthias J. Sax
> Assignee: Hanyu Zheng
> Priority: Major
>
> Calling `StateQueryResult#getOnlyPartitionResult` crashes with an incorrect
> `IllegalArgumentException` if any result is a `FailedQueryResult` (and even
> if there is only a single FailedQueryResult).
> The issue is the internal `filter(r -> r.getResult() != 0)` step, that
> blindly (and incorrectly) calls `getResult`.
> Given the semantics of `getOnlyPartitionResult` we should not care if the
> result is SuccessQueryResult or FailedQueryResult, but only check if there is
> a single result or not. (The user has no means to avoid getting the
> underlying error otherwise.)
> Side-note: why does `FailedQueryResult#getResult` throw an
> IllegalArgumentException (there is no argument passed into the method – it
> should rather be an `IllegalStateException` – but I guess we would need a KIP
> for this fix?)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
