KAFKA-9243 Update the javadocs to include TimestampKeyValueStore#7848
KAFKA-9243 Update the javadocs to include TimestampKeyValueStore#7848miroswan wants to merge 2 commits intoapache:trunkfrom miroswan:KAFKA-9243
Conversation
vvcephei
left a comment
vvcephei
left a comment
There was a problem hiding this comment.
Hey @miroswan , thanks for the PR!
Can you update the PR description with the intended commit message for when we merge this?
Also, I left one comment that seems to apply to all the changes. It might be a good idea to create a quick demo class to try out the APIs and make sure that the code in the javadocs actually compiles and works.
| * ... | ||
| * final String queryableStoreName = table.queryableStoreName(); // returns null if KTable is not queryable | ||
| * ReadOnlyKeyValueStore view = streams.store(queryableStoreName, QueryableStoreTypes.keyValueStore()); | ||
| * TimestampedKeyValueStore view = streams.store(queryableStoreName, QueryableStoreTypes.keyValueStore()); |
There was a problem hiding this comment.
This change doesn't look quite right. It should still be a ReadOnlyKeyValueStore, but the generics are different: ReadOnlyKeyValueStore<K, ValueAndTimestamp<V>>. Also, if the intent is to recommend using the Timestamped variant, then folks would actually have to specify QueryableStoreTypes.timestampedKeyValueStore().
There was a problem hiding this comment.
This this top level class JavaDoc, should we also explain that it it still possible to query the store as plain key-value store only? I can imagine, that not all applications are interested in the timestamp and it would be quite convenient for them to stay with the old pattern (note, that the old pattern is not deprecated and still totally valid to use).
Thoughts?
There was a problem hiding this comment.
@vvcephei Thanks for the heads up. I've dug a little more and was able to confirm that streams.store returns a ReadOnlyKeyValueStore<K, ValueAndTimestamp<V>> when QueryableStoreTypes.timestampedKeyValueStore() is passed as the second argument. It looks like the type returned by streams.store is dependent upon the return type of the create method of that second argument, namely a TimestampedKeyValueStoreType. I'll make the necessary updates and verify that the code in the javadocs works.
@mjsax You raise a good point. We can provide documentation for the two approaches; however, if we add another QueryableStoreType similar to KeyValueStore and TimestampedKeyValueStore then we've got to make more updates in many places. To remedy this, perhaps we only provide a code example of how to query at the KTable interface documentation and remove the examples in the documentation for the methods. Otherwise, we can continue to update these code snippets in the documentation throughout. What do you think?
There was a problem hiding this comment.
Well. I see your point. Maintaining the docs in many different places is quite a challenge. Curious to hear what @vvcephei thinks about this question.
There was a problem hiding this comment.
\cc @vvcephei Any comments?
Btw: There seems to be an overlap with https://issues.apache.org/jira/browse/KAFKA-9290 -- might be worth to resolve both issue with a single PR?
miroswan
changed the title
mjsax
left a comment
mjsax
left a comment
There was a problem hiding this comment.
@miroswan -- sorry for the delay on this PR -- it somehow got dropped.
This PR only update KTable interface, but there are other interfaces that need the same update. Can you fix them, too. For example GlobalKTable, KGroupedStream, TimeWindowedKStream, CogroupedKStream, TimeWindowedCogroupedKStream, and KGroupedTable (maybe there are more...)
| * final String queryableStoreName = table.queryableStoreName(); // returns null if KTable is not queryable | ||
| * ReadOnlyKeyValueStore view = streams.store(queryableStoreName, QueryableStoreTypes.keyValueStore()); | ||
| * ReadOnlyKeyValueStore<String, Long> view = streams.store(queryableStoreName, QueryableStoreTypes.keyValueStore()); | ||
| * view.get(key); |
There was a problem hiding this comment.
nit: can you remove the blanks? Those would be rendered:
// with blanks code would be intendet
view.get(key)
// but we don't want indention
view.get(key);
This applies to all <pre>{@code... sections) -- it was not introduced by you but we should fix it.
|
Closing this PR in favor of #8114 |
3 participants
No description provided.