You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mjsax (via GitHub)" <gi...@apache.org> on 2023/10/14 18:44:28 UTC
[PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
mjsax opened a new pull request, #14550:
URL: https://github.com/apache/kafka/pull/14550
We embrace immutability and thus should return a new object instead of `this`, similar to other config classed we use in the DSL.
Side JavaDocs cleanup for a bunch of classes.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #14550:
URL: https://github.com/apache/kafka/pull/14550#discussion_r1359668981
##########
streams/src/main/java/org/apache/kafka/streams/kstream/ForeachAction.java:
##########
@@ -27,15 +27,18 @@
*
* @param <K> key type
* @param <V> value type
+ *
* @see KStream#foreach(ForeachAction)
*/
public interface ForeachAction<K, V> {
/**
* Perform an action for each record of a stream.
*
- * @param key the key of the record
- * @param value the value of the record
+ * @param key
Review Comment:
It's not necessary but make JavaDocs easier to read in general -- and I like to have consistent formatting across the board -- would like to establish this new formatting if nobody objects.
Especially when there is many parameters with name of different length, it cumbersome to get nice formatting -- adding the description in the next line avoid all those whitespaces (and if we rename a parameter it becomes a single line change and no reformatting is necessary)
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Initializer.java:
##########
@@ -16,12 +16,12 @@
*/
package org.apache.kafka.streams.kstream;
-
/**
* The {@code Initializer} interface for creating an initial value in aggregations.
* {@code Initializer} is used in combination with {@link Aggregator}.
*
- * @param <VA> aggregate value type
+ * @param <VAag> aggregate value type
Review Comment:
Ups.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #14550:
URL: https://github.com/apache/kafka/pull/14550#discussion_r1359669078
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -90,33 +104,43 @@ public static <K, V> Branched<K, V> withConsumer(final Consumer<KStream<K, V>> c
/**
* Create an instance of {@code Branched} with provided chain function and branch name suffix.
*
- * @param chain A function that will be applied to the branch. If the provided function returns
- * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
- * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
- * {@link BranchedKStream} description for details).
- * @param name the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
- * (see {@link BranchedKStream} description for details)
- * @param <K> key type
- * @param <V> value type
+ * @param chain
+ * A function that will be applied to the branch. If the provided function returns
+ * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
+ * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
+ * {@link BranchedKStream} description for details).
+ * @param name
+ * the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
+ * (see {@link BranchedKStream} description for details)
+ *
+ * @param <K> key type
+ * @param <V> value type
+ *
* @return a new instance of {@code Branched}
*/
public static <K, V> Branched<K, V> withFunction(
- final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain, final String name) {
+ final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain,
Review Comment:
Not necessary. It's my personal preferred style. Happy to change it back. Please let me know.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #14550:
URL: https://github.com/apache/kafka/pull/14550#discussion_r1359668737
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -125,21 +149,14 @@ public static <K, V> Branched<K, V> withConsumer(final Consumer<? super KStream<
return new Branched<>(name, null, chain);
}
- /**
- * Create an instance of {@code Branched} from an existing instance.
- *
- * @param branched the instance of {@code Branched} to copy
- */
- protected Branched(final Branched<K, V> branched) {
Review Comment:
Yes.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "guozhangwang (via GitHub)" <gi...@apache.org>.
guozhangwang commented on code in PR #14550:
URL: https://github.com/apache/kafka/pull/14550#discussion_r1359652648
##########
streams/src/main/java/org/apache/kafka/streams/kstream/ForeachAction.java:
##########
@@ -27,15 +27,18 @@
*
* @param <K> key type
* @param <V> value type
+ *
* @see KStream#foreach(ForeachAction)
*/
public interface ForeachAction<K, V> {
/**
* Perform an action for each record of a stream.
*
- * @param key the key of the record
- * @param value the value of the record
+ * @param key
Review Comment:
Is this newline necessary?
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -125,21 +149,14 @@ public static <K, V> Branched<K, V> withConsumer(final Consumer<? super KStream<
return new Branched<>(name, null, chain);
}
- /**
- * Create an instance of {@code Branched} from an existing instance.
- *
- * @param branched the instance of {@code Branched} to copy
- */
- protected Branched(final Branched<K, V> branched) {
Review Comment:
I'm assuming this is just for moving the func around, and removing the javadoc for those protected functions?
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -57,29 +64,36 @@ public static <K, V> Branched<K, V> as(final String name) {
/**
* Create an instance of {@code Branched} with provided chain function.
*
- * @param chain A function that will be applied to the branch. If the provided function returns
- * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
- * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
- * {@link BranchedKStream} description for details).
- * @param <K> key type
- * @param <V> value type
+ * @param chain
+ * A function that will be applied to the branch. If the provided function returns
+ * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
+ * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
+ * {@link BranchedKStream} description for details).
+ *
+ * @param <K> key type
+ * @param <V> value type
+ *
* @return a new instance of {@code Branched}
*/
public static <K, V> Branched<K, V> withFunction(
- final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain) {
+ final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain
Review Comment:
Ditto here?
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Initializer.java:
##########
@@ -16,12 +16,12 @@
*/
package org.apache.kafka.streams.kstream;
-
/**
* The {@code Initializer} interface for creating an initial value in aggregations.
* {@code Initializer} is used in combination with {@link Aggregator}.
*
- * @param <VA> aggregate value type
+ * @param <VAag> aggregate value type
Review Comment:
Agg? :)
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -90,33 +104,43 @@ public static <K, V> Branched<K, V> withConsumer(final Consumer<KStream<K, V>> c
/**
* Create an instance of {@code Branched} with provided chain function and branch name suffix.
*
- * @param chain A function that will be applied to the branch. If the provided function returns
- * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
- * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
- * {@link BranchedKStream} description for details).
- * @param name the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
- * (see {@link BranchedKStream} description for details)
- * @param <K> key type
- * @param <V> value type
+ * @param chain
+ * A function that will be applied to the branch. If the provided function returns
+ * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
+ * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
+ * {@link BranchedKStream} description for details).
+ * @param name
+ * the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
+ * (see {@link BranchedKStream} description for details)
+ *
+ * @param <K> key type
+ * @param <V> value type
+ *
* @return a new instance of {@code Branched}
*/
public static <K, V> Branched<K, V> withFunction(
- final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain, final String name) {
+ final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain,
Review Comment:
Ditto here, I feel these newlines are not very necessary?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax merged PR #14550:
URL: https://github.com/apache/kafka/pull/14550
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix `Consumed` to return new object instead of `this` [kafka]
Posted by "guozhangwang (via GitHub)" <gi...@apache.org>.
guozhangwang commented on code in PR #14550:
URL: https://github.com/apache/kafka/pull/14550#discussion_r1359692144
##########
streams/src/main/java/org/apache/kafka/streams/kstream/Branched.java:
##########
@@ -90,33 +104,43 @@ public static <K, V> Branched<K, V> withConsumer(final Consumer<KStream<K, V>> c
/**
* Create an instance of {@code Branched} with provided chain function and branch name suffix.
*
- * @param chain A function that will be applied to the branch. If the provided function returns
- * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
- * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
- * {@link BranchedKStream} description for details).
- * @param name the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
- * (see {@link BranchedKStream} description for details)
- * @param <K> key type
- * @param <V> value type
+ * @param chain
+ * A function that will be applied to the branch. If the provided function returns
+ * {@code null}, its result is ignored, otherwise it is added to the {@code Map} returned
+ * by {@link BranchedKStream#defaultBranch()} or {@link BranchedKStream#noDefaultBranch()} (see
+ * {@link BranchedKStream} description for details).
+ * @param name
+ * the branch name suffix to be used. If {@code null}, a default branch name suffix will be generated
+ * (see {@link BranchedKStream} description for details)
+ *
+ * @param <K> key type
+ * @param <V> value type
+ *
* @return a new instance of {@code Branched}
*/
public static <K, V> Branched<K, V> withFunction(
- final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain, final String name) {
+ final Function<? super KStream<K, V>, ? extends KStream<K, V>> chain,
Review Comment:
I just personally feel a newline with just an extra param feels not very elegant. Again just my own thoughts, feel free to ignore.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org