KAFKA-6958: Add new NamedOperation interface to enforce consistency i… by fhussonnois · Pull Request #6409 · apache/kafka

fhussonnois · 2019-03-08T19:12:56Z

Hi @mjsax @bbejeck

This is the first PR for the KIP-307. Thanks a lot for the review.

bbejeck · 2019-03-08T20:10:50Z

Thanks @fhussonnois! We'll get started on reviewing these PRs

bbejeck · 2019-03-08T20:35:03Z

\cc @vvcephei and @ableegoldman for reviews

vvcephei

Hey @fhussonnois , just one small request... Otherwise, it's a good start on a long journey of PRs :)

Thanks for the short PR.

bbejeck

Thanks @fhussonnois, I've made a pass and I just have a few minor comments, otherwise, this first PR looks good.

bbejeck · 2019-03-12T18:02:36Z

Java 8 passed Java 11 failed, test results already cleaned up

restest this please

bbejeck · 2019-03-13T22:23:39Z

Java 8 passed, Java 11 failed results already cleaned out

retest this please

mjsax · 2019-03-14T18:30:35Z

Just thinking about is once more: why do we need to make this interface public?

We have Named as public method to use NamedOperation and other public control objects (Consumed etc) that implement it -- but I actually think, users don't need to know about this interface?

\cc @fhussonnois @bbejeck @guozhangwang @vvcephei @ableegoldman

@mjsax you're right about that, currently this interface can be package private. We can still make it public later if needed.

mjsax · 2019-03-14T18:37:36Z

Why does this PR not add NamedOperation to

Produced
Consumed
Printed
Grouped

but only to Joined and Suppressed?

bbejeck · 2019-03-15T15:41:12Z

Java 8 build timed out, Java 11 passed

restest this please

fhussonnois · 2019-03-18T13:45:48Z

@mjsax This PR only add NamedOperation to Joined and Supressed to not introduce new features. It's only about refacorting existing naming methods. Produced Consumed Printed Grouped classes are updated in next PRs.

mjsax · 2019-03-18T18:12:50Z

Thanks @fhussonnois. Understood.

…n naming operations Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) : - add new public interface NamedOperation - deprecate methods Joined.as() and Joined.name() - update Suppredded interface to extend NamedOperation

mjsax

LGTM.

The nit can be ignored or addressed in one of the others PRs. We can also make the interface private in follow ups if we think that's better. Would still be good to get feedback from @bbejeck @vvcephei about this.

mjsax · 2019-03-19T23:50:49Z

        Objects.requireNonNull(joiner, "joiner can't be null");
        Objects.requireNonNull(joined, "joined can't be null");
+        final JoinedInternal<K, V, VO> joinedInternal = new JoinedInternal<>(joined);
+        final String internalName = joinedInternal.name();


nit: should this be name (similar to above) only?

mjsax · 2019-03-20T00:16:28Z

Just looked into #6409 -- it would be helpful if you could add what changes you apply per PR. For example for this PR:

First part of KIP-307:
 - add interface NamedOperation
 - let existing classed extend NameOperation (without changing behavior), ie, Joined, Suppressed
 - does not add new functionality

bbejeck · 2019-03-20T02:16:54Z

I also agree that we can mark the interface private, as we use it to apply a common behavior to the configuration classes and we don't expect users to interact or extend the interface for any other purpose (unless we create a new config object).

However, we can defer the final decision in one of the remaining follow up PR's.

bbejeck · 2019-03-20T02:27:17Z

Merged #6409 into trunk.

* apache/trunk: (23 commits) KAFKA-7986: Distinguish logging from different ZooKeeperClient instances (apache#6493) KAFKA-8102: Add an interval-based Trogdor transaction generator (apache#6444) MINOR: Fix misspelling in protocol documentation KAFKA-8150: Fix bugs in handling null arrays in generated RPC code (apache#6489) KAFKA-8014: Extend Connect integration tests to add and remove workers dynamically (apache#6342) MINOR: Remove line for testing repartition topic name (apache#6488) MINOR: add MacOS requirement to Streams docs MINOR: fix message protocol help text for ElectPreferredLeadersResult (apache#6479) MINOR: list-topics should not require topic param MINOR: Clean up ThreadCacheTest (apache#6485) MINOR: Avoid unnecessary collection copy in MetadataCache (apache#6397) KAFKA-8142: Fix NPE for nulls in Headers (apache#6484) KAFKA-7243: Add unit integration tests to validate metrics in Kafka Streams (apache#6080) MINOR: Add verification step for Streams archetype to Jenkins build (apache#6431) KAFKA-7819: Improve RoundTripWorker (apache#6187) KAFKA-7989: RequestQuotaTest should wait for quota config change before running tests (apache#6482) KAFKA-8098: Fix Flaky Test testConsumerGroups KAFKA-6958: Add new NamedOperation interface to enforce consistency in naming operations (apache#6409) MINOR: capture result timestamps in Kafka Streams DSL tests (apache#6447) MINOR: updated names for deprecated streams constants (apache#6466) ...

…n naming operations (apache#6409) Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) : - add new public interface NamedOperation - deprecate methods Joined.as() and Joined.name() - update Suppredded interface to extend NamedOperation Reviewers: Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

fhussonnois mentioned this pull request Mar 8, 2019

KAFKA-6958: Allow to name operation using parameter classes #6410

Merged

fhussonnois force-pushed the KAFKA-6958-SUB-TASK1 branch from 74f0bf3 to c6319fb Compare March 8, 2019 19:55

bbejeck added the streams label Mar 8, 2019

bbejeck requested review from guozhangwang and mjsax March 8, 2019 20:09

vvcephei requested changes Mar 8, 2019

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/JoinedInternal.java Outdated

bbejeck reviewed Mar 12, 2019

View reviewed changes

mjsax reviewed Mar 14, 2019

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/NamedOperation.java Outdated

mjsax reviewed Mar 14, 2019

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/Suppressed.java Outdated

fhussonnois force-pushed the KAFKA-6958-SUB-TASK1 branch from c6319fb to 27a2be0 Compare March 14, 2019 20:56

fhussonnois force-pushed the KAFKA-6958-SUB-TASK1 branch 2 times, most recently from fdcf66b to f0053e7 Compare March 18, 2019 22:39

fhussonnois force-pushed the KAFKA-6958-SUB-TASK1 branch from f0053e7 to 940484a Compare March 19, 2019 16:09

mjsax approved these changes Mar 19, 2019

View reviewed changes

bbejeck approved these changes Mar 20, 2019

View reviewed changes

bbejeck merged commit fa57eb0 into apache:trunk Mar 20, 2019

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

Conversation

fhussonnois commented Mar 8, 2019

Uh oh!

bbejeck commented Mar 8, 2019

Uh oh!

bbejeck commented Mar 8, 2019

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bbejeck commented Mar 12, 2019

Uh oh!

bbejeck commented Mar 13, 2019

Uh oh!

mjsax Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

fhussonnois Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjsax commented Mar 14, 2019

Uh oh!

bbejeck commented Mar 15, 2019

Uh oh!

fhussonnois commented Mar 18, 2019

Uh oh!

mjsax commented Mar 18, 2019

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax Mar 19, 2019

Choose a reason for hiding this comment

Uh oh!

mjsax commented Mar 20, 2019

Uh oh!

bbejeck commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbejeck commented Mar 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bbejeck commented Mar 20, 2019 •

edited

Loading