ableegoldman commented on pull request #10597: URL: https://github.com/apache/kafka/pull/10597#issuecomment-827302955
>It is a bit strange for me if use StreamsNotStartedException to replace IllegalStateException in the validateIsRunningOrRebalancing() Isn't that what we're doing with the `KafkaStreams#store` method, though? It also invokes `validateIsRunningOrRebalancing()` and thus presumably will throw an IllegalStateException if a user invokes this on a KafkaStreams that hasn't been started. Maybe you can run the test you added with the actual changes commented out and see what gets thrown. >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? Those methods are all, ultimately at least, related to Interactive Queries. My understanding is that they would be used to find out where to route a query for a specific key, for example. Plus they do take a particular store as a parameter, thus even `InvalidStateStore` makes about as much sense for these methods as it does for KafkaStreams#store. Also just fyi, by sheer coincidence it seems this PR will end up landing in 3.0 which is a major version bump and thus (some) breaking changes are allowed. I think changing up exception handling in this way is acceptable (and as noted above it seems we are already doing so for the `#store` case, unless I'm missing something). Plus, it seems like compatibility is a bit nuanced in this particular case. Presumably a user will hit this case just because they forgot to call `start()` before invoking this API, therefore I would expect that any existing apps which are upgraded would already be sure to call start() first and it's only the new applications/new users which are likely to hit this particular exception. That's my read of the situation, anyway. 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
