You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/11/17 21:13:59 UTC
[GitHub] [kafka] fml2 opened a new pull request #9606: doc: Improve JavaDoc for KGroupedStream.aggregate
fml2 opened a new pull request #9606:
URL: https://github.com/apache/kafka/pull/9606
Tell that the store used internally is always a timestamped one.
This is related to KAFKA-10722.
No tests are necessary because only JavaDoc was changed.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#issuecomment-741080765
I've made the changes (rather mechanically) as requested.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate and other similar methods
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r548280588
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedCogroupedKStream.java
##########
@@ -77,7 +77,8 @@
* {@link StreamsConfig#CACHE_MAX_BYTES_BUFFERING_CONFIG cache size}, and
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
Review comment:
Updated
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax merged pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate and other similar methods
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #9606:
URL: https://github.com/apache/kafka/pull/9606
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r525580343
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -381,7 +381,8 @@
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
+ * an internal changelog topic that will be created in Kafka.
Review comment:
Should apply the same improvement to `reduce()` and `count()` overloads? Also for `CogroupedKStream#aggregate()`?
What about `TimeWindowedKStream` and `TimeWindowedCogroupedKStream` ?
Also `StreamsBuilder#table()` (and `#globalTable()`) might need an update?
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -438,7 +439,8 @@
* query the value of the key on a parallel running instance of your Kafka Streams application.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore} -- regardless of what
+ * is specified in the parameter {@materialized}) will be backed by an internal changelog topic that will be created in Kafka.
Review comment:
`{@materialized}` is not valid markup as far as I know. Should we `{@code materialized}`? (same below)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#issuecomment-734766511
???
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate and other similar methods
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r548280702
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedKStream.java
##########
@@ -67,7 +67,8 @@
* {@link StreamsConfig#CACHE_MAX_BYTES_BUFFERING_CONFIG cache size}, and
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
Review comment:
Updated
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r532996901
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -438,7 +439,8 @@
* query the value of the key on a parallel running instance of your Kafka Streams application.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore} -- regardless of what
+ * is specified in the parameter {@materialized}) will be backed by an internal changelog topic that will be created in Kafka.
Review comment:
No need to squash commits. We squash them automatically when merging.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r525882399
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -438,7 +439,8 @@
* query the value of the key on a parallel running instance of your Kafka Streams application.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore} -- regardless of what
+ * is specified in the parameter {@materialized}) will be backed by an internal changelog topic that will be created in Kafka.
Review comment:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r532996684
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -381,7 +381,8 @@
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
+ * an internal changelog topic that will be created in Kafka.
Review comment:
`reduce()` and `count()` are similar to `aggregate()`: reduce only does not allow you to change the value type (ie, output type == input type) and count, well implements an aggregate() that counts :)
And they all work the same for `KGroupedStream`, `CogroupedKStream`, `TimeWindowedKStream` and `TimeWindowedCoGroupedKStream`.
`table()` and `globalTable()` just read a topic and upsert the data into a state store / table.
So it would be great to do them all in one 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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r536328242
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -381,7 +381,8 @@
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
+ * an internal changelog topic that will be created in Kafka.
Review comment:
We can still do multiple PRs -- feel free to open them in parallel reusing the existing JIRA ticket.
Will hold off to merge this because if we can detect other things we need to change in the other PRs, we can keep the applied changes in-sync more easily.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r534476825
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -381,7 +381,8 @@
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
+ * an internal changelog topic that will be created in Kafka.
Review comment:
I'd prefer to commit this chages and create an issue referring to this PR. What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate and other similar methods
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#issuecomment-750589950
Thanks for updating the PR. Merged to `trunk`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate and other similar methods
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r548180364
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedCogroupedKStream.java
##########
@@ -77,7 +77,8 @@
* {@link StreamsConfig#CACHE_MAX_BYTES_BUFFERING_CONFIG cache size}, and
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
Review comment:
For time-windows, it would be a `TimestampedWindowStore` (not tkv-store). (same below for the other method of this class)
Note the signature of `Materialized<K, V, WindowStore<Bytes, byte[]>>` that uses `WindowStore`, not `KeyValueStore`.
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindowedKStream.java
##########
@@ -67,7 +67,8 @@
* {@link StreamsConfig#CACHE_MAX_BYTES_BUFFERING_CONFIG cache size}, and
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
Review comment:
As above. Should we `TimestampedWindowStore` for this class.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r525873831
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -438,7 +439,8 @@
* query the value of the key on a parallel running instance of your Kafka Streams application.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore} -- regardless of what
+ * is specified in the parameter {@materialized}) will be backed by an internal changelog topic that will be created in Kafka.
Review comment:
Yes, actually, I wanted to write this but somehow... I will correct it. Shoudl I then squash the commits?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] fml2 commented on a change in pull request #9606: [KAFKA-10722] doc: Improve JavaDoc for KGroupedStream.aggregate
Posted by GitBox <gi...@apache.org>.
fml2 commented on a change in pull request #9606:
URL: https://github.com/apache/kafka/pull/9606#discussion_r525873511
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/KGroupedStream.java
##########
@@ -381,7 +381,8 @@
* {@link StreamsConfig#COMMIT_INTERVAL_MS_CONFIG commit intervall}.
*
* <p>
- * For failure and recovery the store will be backed by an internal changelog topic that will be created in Kafka.
+ * For failure and recovery the store (which always will be of type {@link TimestampedKeyValueStore}) will be backed by
+ * an internal changelog topic that will be created in Kafka.
Review comment:
Yes, this should be probably done. But since I don't know how they work and what to write there, I'd prefer to do it in another PR (or someone else should do it).
----------------------------------------------------------------
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:
users@infra.apache.org