KAFKA-5876: Apply UnknownStateStoreException for Interactive Queries

[vitojeng]

follow-up #8200

KAFKA-5876's PR break into multiple parts, this PR is part 2 - apply UnknownStateStoreException

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[vitojeng]

@mjsax Sorry for the long pause, please take a look. :)

[mjsax]

Glad to have you back!

I cannot remember the details of the KIP but should we throw this new exception also for allMetadataForStore and queryMetadataForKey?

\cc @vvcephei

[vitojeng]

I cannot remember the details of the KIP but should we throw this new exception also for allMetadataForStore and queryMetadataForKey?

@mjsax In the KIP-216, we talk about all sub types of InvalidStateStoreException. That means this KIP only focus on the existing code that throws InvalidStateStoreException, IMHO. Seems there not have any InvalidStateStoreException thrown in the allMetadataForStore and queryMetadataForKey. So we didn't discuss whether we should throw this exception(UnknownStateStoreException) in the past.

It looks like it will break the API for the new exception. If the answer is 'Yes', should we need another KIP?

[ableegoldman]

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 🙂

[vitojeng]

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 🙂

@ableegoldman Yes. Personally, I hope to complete this KIP's work. 😄

[ableegoldman]

Awesome. I can help get this current PR reviewed and merged from here, and probably 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

[vitojeng]

Awesome. I can help get this current PR reviewed and merged from here, and probably find you someone else to review the next PR(s) since I'm pretty busy 🙂

Great, thanks.

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.

@ableegoldman I just feel that I may need to point out this(will break the API for the new exception).
Your suggestion is great. I'm happy to update the KIP and update the PR for this.

[cadonna]

@vitojeng @mjsax @ableegoldman
My 2 euro cent:
Looking at the code for allMetadataForStore and queryMetadataForKey it seems like the case when a state store is not unknown is not handled by throwing an exception but by returning null or by returning UNKNOWN_HOST. Since it is documented (almost) that way in the javadocs and the relevant KIPs (KIP-67 and KIP-535) do not mention an exception, I think we need a proper update to the existing KIP or a new dedicated KIP for it (both with voting), because it does not seem to be an oversight in the implementation, but rather an oversight in the design of the API. If it were an oversight in then implementation you could argue that users relied on buggy code and a quick update on the KIP thread would be fine. However, with an oversight in the API design, I think a KIP with additions of new methods and deprecations of old methods is needed. If it is an update of the current KIP or a new one in order to not block the existing one is less important (although I am in favor of the latter). WDYT?

[ableegoldman]

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?

[vitojeng]

@cadonna , @ableegoldman

Thanks for great discussion.
I'll rebase trunk and update the streams upgrade guide.

[ableegoldman]

Alright, LGTM. @vitojeng please ping me when the PR build is done and I'll merge

[vitojeng]

@ableegoldman
The PR build is done, please talk a look.

[ableegoldman]

Just two unrelated test failures in the build:
connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication()
kafka.integration.MetricsDuringTopicCreationDeletionTest.testMetricsDuringTopicCreateDelete()

[ableegoldman]

Merged to trunk, ready for the next PR 🙂

[vitojeng]

Merged to trunk, ready for the next PR 🙂

Will do, thanks @ableegoldman !
:)