vitojeng commented on pull request #10597: URL: https://github.com/apache/kafka/pull/10597#issuecomment-827296663
> LGTM, just some suggestions for the wording. Can we add a note to the section in the upgrade-guide that you added for the last exception? Sure, will do. Thank for your reminder. > By the way, I wonder if we should also throw this exception for the `allMetadata, `allMetadataForStore`, and `queryMetadataForKey`methods? It seems these methods along with`KafkaStreams#store`all do the same thing of calling`validateIsRunningOrRebalancing` which then checks on the KafkaStreams state and throws IllegalStateException if not in one of those two states. Imo all of these would benefit from the StreamsNotStartedException and it doesn't make sense to single that one out. One thing may need remind, in the KIP-216, `StreamsNotStartedException` is sub class of `InvalidStateStoreException`. That means `StreamsNotStartedException` is an exception related to the StateStore. It is a bit strange for me if use `StreamsNotStartedException` to replace `IllegalStateException` in the `validateIsRunningOrRebalancing()`. > Thoughts? I know it's not in the KIP but in this case it seems like a trivial improvement that would merit just a quick update on the KIP discussion thread but not a whole new KIP of its own I wonder if it worth to break the API compatibility? The trivial improvement will break the API compatibility(`allMetadata()`, `allMetadataForStore()`, `queryMetadataForKey`). It seems this change means that KIP-216 is no longer API Compatibility and the scope of the KIP will exceed Interactive Query related. WDYT? -- 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
