MINOR: factor state checks into descriptive methods and clarify javadocs#11123
Conversation
| return equals(NOT_RUNNING) || equals(ERROR); | ||
| } | ||
|
|
||
| public boolean hasStartedOrFinishedShuttingDown() { |
There was a problem hiding this comment.
Technically this one is not used anywhere yet, but I intend to use it in #10788 so I just went ahead and added it here. Happy to remove it until it's actually needed if anyone prefers
There was a problem hiding this comment.
Maybe have it be isShuttingDown() || hasStartedOrFinishedShuttingDown() to be less brittle?
| } | ||
| } else { | ||
| throw new IllegalStateException("Can only set UncaughtExceptionHandler in CREATED state. " + | ||
| throw new IllegalStateException("Can only set UncaughtExceptionHandler before calling start(). " + |
There was a problem hiding this comment.
I feel we've had a number of users be confused by this error message before, because they don't know what it actually means for the state to be CREATED -- a more useful exception would be to describe what they did wrong/need to do right to fix this
|
Call for review on this minor cleanup PR to make the state handling a bit more clear and (hopefully) safe -- @wcarlson5 @mjsax @lct45 @cadonna @showuon |
showuon
left a comment
showuon
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the refactor!
| * @throws StreamsNotStartedException If Streams state is {@link KafkaStreams.State#CREATED CREATED}. Just | ||
| * retry and wait until to {@link KafkaStreams.State#RUNNING RUNNING}. | ||
| * @throws StreamsNotStartedException If Streams has not yet been started. Just call {@link KafkaStreams#start()} | ||
| * and then retry this call. |
There was a problem hiding this comment.
Nice catch! We actually allow REBALANCING and RUNNING state in this method.
| } | ||
| } else { | ||
| throw new IllegalStateException("Can only set UncaughtExceptionHandler in CREATED state. " + | ||
| throw new IllegalStateException("Can only set UncaughtExceptionHandler before calling start(). " + |
| * Calling this method triggers a restore of local {@link StateStore}s on the next {@link #start() application start}. | ||
| * | ||
| * @throws IllegalStateException if this {@code KafkaStreams} instance is currently {@link State#RUNNING running} | ||
| * @throws IllegalStateException if this {@code KafkaStreams} instance has been started and hasn't fully shut down |
| return equals(NOT_RUNNING) || equals(ERROR); | ||
| } | ||
|
|
||
| public boolean hasStartedOrFinishedShuttingDown() { |
There was a problem hiding this comment.
Maybe have it be isShuttingDown() || hasStartedOrFinishedShuttingDown() to be less brittle?
…ocs (apache#11123) Just a bit of minor cleanup that (a) does some prepwork for another PR I'm working on, (b) updates the javadocs & exception messages to report a more useful error to the user and describe what they actually need to do, and (c) hopefully makes these state checks more future-proof by defining methods for each kind of check in one place that can be easily updated instead of tracking down every individual check. Reviewers: Walker Carlson <wcarlson@confluent.io>, Luke Chen <showuon@gmail.com>
3 participants
Just a bit of minor cleanup that (a) does some prepwork for another PR I'm working on, (b) updates the javadocs & exception messages to report a more useful error to the user and describe what they actually need to do, and (c) hopefully makes these state checks more future-proof by defining methods for each kind of check in one place that can be easily updated instead of tracking down every individual check.