[GitHub] [kafka] ableegoldman commented on pull request #9821: KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries
ableegoldman commented on pull request #9821: URL: https://github.com/apache/kafka/pull/9821#issuecomment-825863470 Merged to trunk, ready for the next PR 🙂 -- 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
[GitHub] [kafka] ableegoldman commented on pull request #9821: KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries
ableegoldman commented on pull request #9821: URL: https://github.com/apache/kafka/pull/9821#issuecomment-825862826 Just two unrelated test failures in the build: `connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()` `kafka.integration.MetricsDuringTopicCreationDeletionTest.testMetricsDuringTopicCreateDelete()` -- 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
[GitHub] [kafka] ableegoldman commented on pull request #9821: KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries
ableegoldman commented on pull request #9821: URL: https://github.com/apache/kafka/pull/9821#issuecomment-822625233 Fine with me. It does seem odd that we'd be inconsistent, but I don't think we need to solve everything with this one KIP/PR. Let's just file a ticket for this so we don't forget. (FWIW I think it was neither an oversight nor an intentional design, at least in the case of `queryMetadataForKey`, since this KIP-535 was started and completed while this KIP was still in progress, and the author most likely wasn't aware that we had plans to improve the IQ exceptions.) I think in that case, this PR covers everything it needs to. @vitojeng Can you just add a quick note about this new exception to the streams upgrade guide under the section for API changes in 3.0? -- 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
[GitHub] [kafka] ableegoldman commented on pull request #9821: KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries
ableegoldman commented on pull request #9821: URL: https://github.com/apache/kafka/pull/9821#issuecomment-819206522 Awesome. I can help get this current PR reviewed and merged from here, and find you someone else to review the next PR(s) since I'm pretty busy 🙂 Now, regarding your last question on this PR: am I reading it correctly that we just don't ever throw `InvalidStateStoreException` from `allMetadataForStore` or `queryMetadataForKey` at the moment, therefore it doesn't make sense to throw UnknownStateStoreException from these methods as part of this KIP? Personally, I think it's ok to just throw whatever exception makes sense from wherever in the code it makes sense to do so. You can send a quick update note to the KIP thread to say that you're making this amendment, and if anyone has a concern they can respond there. For this specific case: it certainly seems to make sense that `allMetadataForStore` would throw an UnknownStateStoreException if a store name was passed in that's, well, unknown. I do see that we don't currently throw anything like that, and it looks like we would just return nothing if an invalid store was passed in, but that was probably just an oversight -- I don't see why it should behave any differently from `KafkaStreams#store`. Same goes for `queryMetadataForKey` -- 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
[GitHub] [kafka] ableegoldman commented on pull request #9821: KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries
ableegoldman commented on pull request #9821: URL: https://github.com/apache/kafka/pull/9821#issuecomment-819180159 Hey @mjsax and @vitojeng , are you still interested in this? I haven't been following this KIP myself but I think the IQ exceptions are a valuable addition to Streams, and it would be nice to have this KIP finally pushed across the finish line 🙂 -- 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
