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 2021/03/28 02:02:51 UTC
[GitHub] [kafka] guozhangwang opened a new pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
guozhangwang opened a new pull request #10421:
URL: https://github.com/apache/kafka/pull/10421
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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] guozhangwang merged pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #10421:
URL: https://github.com/apache/kafka/pull/10421
--
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] vvcephei commented on a change in pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #10421:
URL: https://github.com/apache/kafka/pull/10421#discussion_r609080989
##########
File path: streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java
##########
@@ -725,12 +725,11 @@ public void shouldUseSpecifiedNameForLeftJoinOperationBetweenKStreamAndKStream()
}
@Test
- @Deprecated
public void shouldUseGeneratedStoreNamesForLeftJoinOperationBetweenKStreamAndKStream() {
final KStream<String, String> streamOne = builder.stream(STREAM_TOPIC);
final KStream<String, String> streamTwo = builder.stream(STREAM_TOPIC_TWO);
- streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), Joined.as(STREAM_OPERATION_NAME));
+ streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), StreamJoined.with(Serdes.String(), Serdes.String(), Serdes.String()).withName(STREAM_OPERATION_NAME));
Review comment:
Oh, I see.
The docs on `Joined.as` say: "Create an instance of {@code Joined} with base name for all components of the join, this may include any repartition topics created to complete the join"
`StreamJoined#withName` says something similar: "Set the name to use for the join processor and the repartition topic(s) if required"
Whereas `StreamJoined.as` says: "Creates a {@link StreamJoined} instance using the provided name for the state stores and hence the changelog topics for the join stores")
So, it sounds like we realized that we'd accidentally forgotten to name the stores as part of `Joined.as` and `StreamJoined.withName` and patched it up in `StreamJoined.as`.
But of course, we couldn't just fix it in-place, so we wound up with this wacky combination. I'm honestly not sure the best way to proceed. I doubt very many people are going to figure out what you figured out. Maybe we should just add a really verbose javadoc on `StreamJoined.as` explaining what they need to do if they're coming from `Joined.as`?
--
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] guozhangwang commented on a change in pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #10421:
URL: https://github.com/apache/kafka/pull/10421#discussion_r609160610
##########
File path: streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java
##########
@@ -725,12 +725,11 @@ public void shouldUseSpecifiedNameForLeftJoinOperationBetweenKStreamAndKStream()
}
@Test
- @Deprecated
public void shouldUseGeneratedStoreNamesForLeftJoinOperationBetweenKStreamAndKStream() {
final KStream<String, String> streamOne = builder.stream(STREAM_TOPIC);
final KStream<String, String> streamTwo = builder.stream(STREAM_TOPIC_TWO);
- streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), Joined.as(STREAM_OPERATION_NAME));
+ streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), StreamJoined.with(Serdes.String(), Serdes.String(), Serdes.String()).withName(STREAM_OPERATION_NAME));
Review comment:
Thanks! That makes sense and brought up some vague memories of mine as well.
I will add a bit more javadoc on `StreamJoined` then.
--
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] guozhangwang commented on a change in pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #10421:
URL: https://github.com/apache/kafka/pull/10421#discussion_r602817335
##########
File path: streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java
##########
@@ -725,12 +725,11 @@ public void shouldUseSpecifiedNameForLeftJoinOperationBetweenKStreamAndKStream()
}
@Test
- @Deprecated
public void shouldUseGeneratedStoreNamesForLeftJoinOperationBetweenKStreamAndKStream() {
final KStream<String, String> streamOne = builder.stream(STREAM_TOPIC);
final KStream<String, String> streamTwo = builder.stream(STREAM_TOPIC_TWO);
- streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), Joined.as(STREAM_OPERATION_NAME));
+ streamOne.leftJoin(streamTwo, (value1, value2) -> value1, JoinWindows.of(Duration.ofHours(1)), StreamJoined.with(Serdes.String(), Serdes.String(), Serdes.String()).withName(STREAM_OPERATION_NAME));
Review comment:
While refactoring the tests, I found that `StreamJoined` static constructors only allows initializing with `storeName`, but not with `name` (used for operator names). E.g. here I'd have to use the `with` constructor then specify `withName`. Is this by-design? cc @vvcephei
--
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] guozhangwang commented on pull request #10421: KAFKA-12568: Remove deprecated APIs in KStream, KTable and Joined
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #10421:
URL: https://github.com/apache/kafka/pull/10421#issuecomment-808937689
Bumped into https://issues.apache.org/jira/browse/KAFKA-12557, there's a PR fixing it already.
--
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