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/11 03:06:11 UTC
[GitHub] [kafka] mjsax opened a new pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
mjsax opened a new pull request #10300:
URL: https://github.com/apache/kafka/pull/10300
ProcessorContext#forward was changed via KIP-251 in 2.0.0 release. This PR removes the old and deprecated overloads.
Call for review @guozhangwang @ableegoldman
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r592837536
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/AbstractProcessorContextTest.java
##########
@@ -155,29 +155,20 @@ public void shouldReturnEmptyHeadersIfHeadersAreNotSet() {
assertThat(context.headers(), is(emptyIterable()));
}
- @Test
- public void shouldThrowIllegalStateExceptionOnHeadersIfNoRecordContext() {
Review comment:
This test was broken (missing `fail()` statement in the try-catch block).
The test should have been removed via https://github.com/apache/kafka/pull/9361 though.
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #10300:
URL: https://github.com/apache/kafka/pull/10300
--
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593538979
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
##########
@@ -280,61 +280,6 @@ public void testDrivingSimpleTopology() {
assertTrue(outputTopic1.isEmpty());
}
-
- @Test
- public void testDrivingMultiplexingTopology() {
Review comment:
It's using the "old API" topology provided by `createMultiplexingTopology` (and below `createMultiplexByNameTopology`), ie, `MultiplexByNameProcessor` and `MultiplexingProcessor`.
I did double check and the tested multiplexing case is tested using the new API via `createMultiProcessorTimestampTopology` that is using the `FanOutTimestampProcessor` that is the "new" equivalent of the old (and removed) `MultiplexByNameProcessor` processor (for the multiplex by index we don't need a new test as we removed routing by index on purpose and it's not part of the new API any longer.)
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#issuecomment-797159492
@ableegoldman Remove the failing test -- cf inline comment.
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593538020
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/AbstractProcessorContextTest.java
##########
@@ -155,29 +155,20 @@ public void shouldReturnEmptyHeadersIfHeadersAreNotSet() {
assertThat(context.headers(), is(emptyIterable()));
}
- @Test
- public void shouldThrowIllegalStateExceptionOnHeadersIfNoRecordContext() {
Review comment:
@ableegoldman Follow up though: should we keep this test using reverted logic: `shouldReturnEmptyHeaderIfRecordContextIsNull` ?
----------------------------------------------------------------
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] ableegoldman commented on a change in pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593468044
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
##########
@@ -280,61 +280,6 @@ public void testDrivingSimpleTopology() {
assertTrue(outputTopic1.isEmpty());
}
-
- @Test
- public void testDrivingMultiplexingTopology() {
Review comment:
Just wondering, why do we remove this test (and the 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] mjsax commented on a change in pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r592035201
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/ProcessorContext.java
##########
@@ -189,29 +189,6 @@ Cancellable schedule(final Duration interval,
*/
<K, V> void forward(final K key, final V value, final To to);
- /**
- * Forwards a key/value pair to one of the downstream processors designated by childIndex.
- *
- * @param key key
- * @param value value
- * @param childIndex index in list of children of this node
- * @deprecated please use {@link #forward(Object, Object, To)} instead
- */
- // TODO when we remove this method, we can also remove `ProcessorNode#children`
Review comment:
Seems this comment is outdated (cf `ProcessorNode` in this 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] ableegoldman commented on a change in pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593553762
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/AbstractProcessorContextTest.java
##########
@@ -155,29 +155,20 @@ public void shouldReturnEmptyHeadersIfHeadersAreNotSet() {
assertThat(context.headers(), is(emptyIterable()));
}
- @Test
- public void shouldThrowIllegalStateExceptionOnHeadersIfNoRecordContext() {
Review comment:
Just want to make sure I understand the background here: as of KIP-478 we no longer throw if there's no active RecordContext, but instead just return empty. Is that right? If so then yes, an updated test like `shouldReturnEmptyHeaderIfRecordContextIsNull` seems good to have
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r592837536
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/AbstractProcessorContextTest.java
##########
@@ -155,29 +155,20 @@ public void shouldReturnEmptyHeadersIfHeadersAreNotSet() {
assertThat(context.headers(), is(emptyIterable()));
}
- @Test
- public void shouldThrowIllegalStateExceptionOnHeadersIfNoRecordContext() {
Review comment:
This test was broken (missing `fail()` statement in the try-catch block).
The test should have been removed via https://github.com/apache/kafka/pull/9361 though -- the PR did not catch it because the test was broken.
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593539158
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java
##########
@@ -37,7 +37,6 @@
public class ProcessorNode<KIn, VIn, KOut, VOut> {
- // TODO: 'children' can be removed when #forward() via index is removed
Review comment:
Cool. We can still change it later as it's only internal stuff if we feel the need.
----------------------------------------------------------------
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] ableegoldman commented on a change in pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r593468505
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java
##########
@@ -37,7 +37,6 @@
public class ProcessorNode<KIn, VIn, KOut, VOut> {
- // TODO: 'children' can be removed when #forward() via index is removed
Review comment:
Fine with me
----------------------------------------------------------------
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] ableegoldman commented on pull request #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#issuecomment-797026073
Looks like a relevant test failure: `AbstractProcessorContextTest failed`
----------------------------------------------------------------
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 #10300: KAFKA-12452: Remove deprecated overloads of ProcessorContext#forward
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10300:
URL: https://github.com/apache/kafka/pull/10300#discussion_r592036244
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java
##########
@@ -37,7 +37,6 @@
public class ProcessorNode<KIn, VIn, KOut, VOut> {
- // TODO: 'children' can be removed when #forward() via index is removed
Review comment:
`children` is actually used again in the meantime and we should keep it.
(It's of course somewhat redundant to `childByName` but is also preserves the order of the nodes -- using a `TreeMap` might work however, as we return a `List` of child name form one method it would imply extra work to extract the keys...)
----------------------------------------------------------------
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