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.

Sign up for GitHub

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

Jump to bottom

KAFKA-4217: Add KStream.flatTransform #5273

Merged
merged 16 commits into from Jan 27, 2019
Merged

KAFKA-4217: Add KStream.flatTransform #5273

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cc4a397
Add KStream.flatTransform
Jun 15, 2018
e220883
Add KStream.flatTransform (push 2)
cadonna Sep 10, 2018
bb0e6cc
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Sep 10, 2018
8380808
Add KStream.flatTransform (push 3)
cadonna Sep 23, 2018
ab703f8
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Sep 23, 2018
f90c38c
Add KStream.flatTransform (push 4)
cadonna Sep 23, 2018
bd8410e
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Oct 7, 2018
e4e3eed
Add KStream.flatTransform (push 5)
cadonna Oct 9, 2018
a1e1ccf
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Nov 17, 2018
ec27fbc
Add KStream.flatTransform (push 6)
cadonna Nov 17, 2018
0463abc
Add KStream.flatTransform (push 7)
cadonna Dec 29, 2018
a432cd1
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Dec 29, 2018
a78be14
Add KStream.flatTransform (push 8)
cadonna Dec 30, 2018
4002654
Add KStream.flatTransform (push 9)
cadonna Jan 21, 2019
4a6b14a
Merge remote-tracking branch 'upstream/trunk' into kafka4217-flatTran…
cadonna Jan 21, 2019
defa401
Add KStream.flatTransform (push 10)
cadonna Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java
View file
Open in desktop
Expand Up @@ -263,6 +263,7 @@ public interface KStream<K, V> {
* @see #mapValues(ValueMapperWithKey)
* @see #flatMapValues(ValueMapper)
* @see #flatMapValues(ValueMapperWithKey)
* @see #transform(TransformerSupplier, String...)
* @see #flatTransform(TransformerSupplier, String...)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove transform ? We only add a new flatTransform but transform is not removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to make the list of referenced methods too long, so I thought to let map reference transform and flatMap reference flatTransform. But I do not have hard feelings about it.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. Not sure either :) So far, we list a lot of methods. Do you think we should do a general cleanup reduction of listed method? In the past, we only added but never removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of either just adding the new method to the list (without removing another one) or deleting this whole list.
Personally, I feel the list is a little redundant with this interface itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with both options, but I am in favor of deleting the list. Out of curiosity, what is the advantage of just adding new methods to the list without removing others?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless @mjsax objects, I vote to just delete these lists. At this point, it almost looks like it's directing you to all the other methods in this interface, which seems redundant.

I'm not sure I follow your last question. The list exists to direct readers to other relevant methods. I'm not sure why adding flatTransform renders transform irrelevant...

Copy link
Member

Choose a reason for hiding this comment

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

I think, it's overall useful to use @see to guide users -- however, I agree that it's getting a very long list. This seems to be a general issue though. Maybe, we should do a follow up PR, and go over all JavaDocs and clean them up? We should define a "strategy/guideline" how we cross reference. For example, does it make sense to link to both flatMapValues -- I guess not.

WDYT @vvcephei ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm fine with defining and applying a coherent strategy.

In lieu of that, I guess the default thing to do here would be to just add the new method to the list without removing any other items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-added the links. @vvcephei My questions was about just adding links vs reorganizing links. I think my question was answered by the decision taken.

Copy link
Member

Choose a reason for hiding this comment

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

Created https://issues.apache.org/jira/browse/KAFKA-7602

* @see #transformValues(ValueTransformerSupplier, String...)
* @see #transformValues(ValueTransformerWithKeySupplier, String...)
Expand Down Expand Up @@ -304,6 +305,7 @@ public interface KStream<K, V> {
* @see #flatMap(KeyValueMapper)
* @see #mapValues(ValueMapper)
* @see #mapValues(ValueMapperWithKey)
* @see #transform(TransformerSupplier, String...)
* @see #flatTransform(TransformerSupplier, String...)
* @see #transformValues(ValueTransformerSupplier, String...)
* @see #transformValues(ValueTransformerWithKeySupplier, String...)
Expand Down Expand Up @@ -351,6 +353,7 @@ public interface KStream<K, V> {
* @see #flatMap(KeyValueMapper)
* @see #mapValues(ValueMapper)
* @see #mapValues(ValueMapperWithKey)
* @see #transform(TransformerSupplier, String...)
* @see #flatTransform(TransformerSupplier, String...)
* @see #transformValues(ValueTransformerSupplier, String...)
* @see #transformValues(ValueTransformerWithKeySupplier, String...)
Expand Down Expand Up @@ -497,7 +500,7 @@ void to(final TopicNameExtractor<K, V> topicExtractor,
* returns zero or one output record.
* Thus, an input record {@code <K,V>} can be transformed into an output record {@code <K':V'>}.
* This is a stateful record-by-record operation (cf. {@link #map(KeyValueMapper)}).
* Furthermore, via {@link org.apache.kafka.streams.processor.Punctuator#punctuate(long)} the processing progress
* Furthermore, via {@link org.apache.kafka.streams.processor.Punctuator#punctuate(long)}, the processing progress
* can be observed and additional periodic actions can be performed.
*
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already changed by somebody else.

* <p>
Expand Down