Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: cleanup KStream JavaDocs (7/N) - repartition/to/toTable #18760

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Jan 31, 2025

No description provided.

Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I left some comments

* The topic names for each record to send to is dynamically determined based on the {@link TopicNameExtractor}.
* Materialize the record of this stream to different topics.
* The provided {@link TopicNameExtractor} is applied to each input record to compute the output topic name.
* All topics should be manually created before they are use (i.e., before the Kafka Streams application is started).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* All topics should be manually created before they are use (i.e., before the Kafka Streams application is started).
* All topics should be manually created before they are used (i.e., before the Kafka Streams application is started).

*
* @param topicExtractor the extractor to determine the name of the Kafka topic to write to for each record
* @param produced the options to use when producing to the topic
* See {@link #to(TopicNameExtractor)}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter produced is not explained anywhere now. Same for the other methods. Would it make sense to refer from most generic to the most specific overload instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's covered above on to(String) as well as to(TopicNameExtractor):

<p>To explicitly set key/value serdes or the partitioning strategy, use {@link #to(String, Produced)}.

Not sufficient, or did you miss it?

Would it make sense to refer from most generic to the most specific overload instead?

I did consider this originally, but found if overall more complicated than helpful, and though it's easier to describe the most simple overload and just add "forward reference" (as quoted above, and used elsewhere) instead.

* by Kafka Streams.
*
* <p>Note: If the result {@link KTable} is materialized, it is not possible to apply
* {@link StreamsConfig#REUSE_KTABLE_SOURCE_TOPICS "source topic optimization"}, because
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of this syntax. Do you know / have you checked that it works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not intent to make it syntax, but just regular text with a link... If that's confusing, happy to change it.

@mjsax mjsax force-pushed the minor-update-kstreams-javadocs-7 branch from 283be2d to 2142d88 Compare February 5, 2025 04:58
Copy link
Member

@lucasbru lucasbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mjsax mjsax merged commit bdab927 into apache:trunk Feb 6, 2025
7 of 9 checks passed
@mjsax mjsax deleted the minor-update-kstreams-javadocs-7 branch February 6, 2025 18:57
@chia7712
Copy link
Member

This PR is great and has given me an idea for improving our CI process.

This PR introduce a java doc error, and hence QA does not run streams:test.

2025-02-05T05:09:25.4676334Z /home/runner/work/kafka/kafka/streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java:694: warning: invalid usage of tag {@link #to(String).
2025-02-05T05:09:25.4677773Z 
2025-02-05T05:09:25.4692001Z      * See {@link #to(String).}
2025-02-05T05:09:25.4692476Z > Task :streams:javadoc FAILED
2025-02-05T05:09:25.4693167Z            ^
2025-02-05T05:09:25.4694441Z /home/runner/work/kafka/kafka/streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java:694: warning: invalid usage of tag {@link #to(String).
2025-02-05T05:09:25.4695730Z      * See {@link #to(String).}
2025-02-05T05:09:25.4696252Z            ^
2025-02-05T05:09:25.4697406Z /home/runner/work/kafka/kafka/streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java:694: warning: invalid usage of tag {@link #to(String).
2025-02-05T05:09:25.4698737Z      * See {@link #to(String).}
2025-02-05T05:09:25.4699299Z            ^

By contrast, quarantinedTest is still executable because quarantinedTest does not depend on javadoc

https://github.com/apache/kafka/blob/trunk/build.gradle#L710

Although #18838 resolves the current doc error, I believe we should enhance our CI to prevent similar issues from occurring in the future. To be honest, it is too hard to request reviewers to check all contents of CI. Maybe we can add javadoc to the build phase which is similar to #18183. @mumrah WDYT?

@mjsax
Copy link
Member Author

mjsax commented Feb 10, 2025

Thanks for highlighting this issue (and thanks for the PR to fix it). The build for this PR did not surface this issue (otherwise we would not have merged it). I agree, that it might be good to fail PR builds if there is JavaDocs errors.

pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025
manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants