ableegoldman commented on a change in pull request #10597:
URL: https://github.com/apache/kafka/pull/10597#discussion_r621436955
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -94,7 +94,14 @@ <h1>Upgrade Guide and API Changes</h1>
<h3><a id="streams_api_changes_300"
href="#streams_api_changes_300">Streams API changes in 3.0.0</a></h3>
<p>
- A new exception may be thrown from <code>KafkaStreams#store()</code>.
If the specified store name does not exist in the topology, an
UnknownStateStoreException will be thrown instead of the former
InvalidStateStoreException. See <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-216%3A+IQ+should+throw+different+exceptions+for+different+errors";>KIP-216</a>
for more information.
+ Interactive Query may throw different new exceptions for different
errors:
Review comment:
```suggestion
Interactive Queries may throw new exceptions for different errors:
```
##########
File path: docs/streams/upgrade-guide.html
##########
@@ -94,7 +94,14 @@ <h1>Upgrade Guide and API Changes</h1>
<h3><a id="streams_api_changes_300"
href="#streams_api_changes_300">Streams API changes in 3.0.0</a></h3>
<p>
- A new exception may be thrown from <code>KafkaStreams#store()</code>.
If the specified store name does not exist in the topology, an
UnknownStateStoreException will be thrown instead of the former
InvalidStateStoreException. See <a
href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-216%3A+IQ+should+throw+different+exceptions+for+different+errors";>KIP-216</a>
for more information.
+ Interactive Query may throw different new exceptions for different
errors:
+ </p>
+ <ul>
+ <li> <code>UnknownStateStoreException</code>: If the specified store
name does not exist in the topology, an <code>UnknownStateStoreException</code>
will be thrown instead of the former
<code>InvalidStateStoreException</code>.</li>
+ <li> <code>StreamsNotStartedException</code>: If Streams state is not
<code>REBALANCING</code> or <code>REBALANCING</code>, an
<code>StreamsNotStartedException</code> will be thrown instead of the former
<code>IllegalStateException</code>.</li>
Review comment:
```suggestion
<li> <code>StreamsNotStartedException</code>: If Streams state is
not <code>REBALANCING</code> or <code>REBALANCING</code>, a
<code>StreamsNotStartedException</code> will be thrown instead of the former
<code>IllegalStateException</code>.</li>
```
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -344,7 +345,7 @@ private boolean isRunningOrRebalancing() {
private void validateIsRunningOrRebalancing() {
if (!isRunningOrRebalancing()) {
- throw new IllegalStateException("KafkaStreams is not running.
State is " + state + ".");
+ throw new StreamsNotStartedException("KafkaStreams is not running.
State is " + state + ".");
Review comment:
It seems a bit odd to throw a StreamsNotStartedException in case the
KafkaStreams is not in RUNNING or REBALANCING. That seems to imply we would
throw a a StreamsNotStartedException if the KafkaStreams had indeed been
started, but then crashed or was closed to that the state is now one of the
PENDING_{SHUTDOWN/ERROR} or NOT_RUNNING/ERROR.
The thing I'm wondering about is whether we should (a) adopt yet another
exception to cover this case specifically (eg StreamsAlreadyClosedException or
something), or (b) change the name of StreamsNotStarted to be a bit more
generic, eg StreamsNotRunningException, and describe which state specifically
in the error message of the exception, or (c) continue throwing
StreamsNotStartedException only when the state is CREATED, and just continue to
throw IllegalStateException if the state is one of the closed states I listed
above.
Personally I think (c) makes the most sense here: then we don't need to
update the KIP (other than to clarify we'll throw this for the other
metadataForKey, etc methods in addition to #store). But mainly because
IllegalStateException actually does seem appropriate for all state other than
CREATED or RUNNING or REBALANCING -- all those other states are either
terminal, or transition into a terminal state, so there's basically no way to
recover and retry at this point. You'd need to bounce the app most likely. So
from a user perspective you would want to catch and retry maybe on
StreamsNotStartedException, but IllegalStateException maybe even should in fact
kill the app so it can be restarted (eg k8s restarts the process/pod). I'm not
sure we'd want to introduce another exception where the handling would be
pretty much identical to not handling it at all. But I can be convinced, and
maybe you have other ideas or opinions I haven't considered yet. Thoughts?
--
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:
us...@infra.apache.org
