KAFKA-7502: Cleanup KTable materialization logic in a single place by dongjinleekr · Pull Request #6174 · apache/kafka

dongjinleekr · 2019-01-19T11:01:21Z

This is a draft cleanup for KAFKA-7502. Here is the details:

Make KTableKTableJoinNode abstract, and define its child classes ([NonMaterialized,Materialized]KTableKTableJoinNode) instead: now, all materialization-related routines are separated into the other classes.
KTableKTableJoinNodeBuilder#build now instantiates [NonMaterialized,Materialized]KTableKTableJoinNode classes instead of KTableKTableJoinNode.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

dongjinleekr · 2019-01-19T11:02:28Z

@guozhangwang Is this what you meant? I found similar routines in KTableImpl#[filter, map, transform] methods. If it is correct, I will do the same work to that methods.

Please have a look you are free.

dongjinleekr · 2019-01-30T05:32:52Z

retest this please

dongjinleekr · 2019-02-22T13:47:46Z

Rebased against the latest trunk. cc/ @guozhangwang @mjsax

guozhangwang

@dongjinleekr Thanks for your PR! I went through it and realized my original description on the JIRA ticket may not be very clear. I've hence updated the description and let me know if it clarifies a few things.

As for the PR itself, splitting KTableKTableJoinNode seems to me not addressing the original issue, but I do really like the way you inlined buildJoin. So how about reducing the scope of this PR to just do the cleanup of inlining buildJoin, while we can continue discussing on the ticket at the best solution to achieve the address of the ticket.

dongjinleekr · 2019-03-04T14:46:12Z

@guozhangwang I see. Then, how about this? I rebased against the latest trunk and added two commits (296ddfc, e57ad22); Now, all materialization logic in KTableKTableJoinNode is now moved into KTableImpl#doJoin. Is this way correct?

guozhangwang

This PR lgtm. I think this is a good first step towards KAFKA-7502. Left some minor comments.

Call for another review @bbejeck

guozhangwang · 2019-03-06T17:18:17Z

@dongjinleekr could you run the unit test locally and see if the error is related?

guozhangwang · 2019-03-06T17:18:22Z

retest this please

bbejeck

Overall this looks good to me, just a couple of minor comments.

bbejeck · 2019-03-07T16:25:14Z

Java 8 passed, Java 11 failed but build results already cleaned up.

retest this please

bbejeck · 2019-03-08T15:59:03Z

Both Java 8 and Java 11 failed, test results already cleaned out

retest this please

bbejeck · 2019-03-11T15:16:16Z

@dongjinleekr what's the status on this PR? Once the comments are addressed I believe we can merge this PR.

dongjinleekr · 2019-03-12T14:24:18Z

@bbejeck Here is the update for the join operation. I am now working on the other operations and can finish it by this Friday.

guozhangwang · 2019-03-12T16:50:05Z

@dongjinleekr Let's use separate PRs on other operations so that we can keep this one small and tangible for reviewing / merging.

BTW the failures are due to compilation errors: Execution failed for task ':streams:compileJava'.

Could you run ./gradlew streams:test locally and see if you can resolve these errors?

…ized]KTableKTableJoinNode. 2. Add KTableKTableJoinNodeBuilde#builder.

1. Add KTableKTableJoinNode[Builder]#keySerde. 2. Reorder KTableKTableJoinNodeBuilder's member methods, following the memeber variable definition order. 3. Add KTableKTableJoinNode#[getKeySerde, getValueSerde] methods. 4. Move KTableImpl#[keySerde, valueSerde] decision logic into KTableKTableJoinNodeBuilder#builder and [NonMaterialized, Materialized]KTableKTableJoinNode#[keySerde, valueSerde].

…e 'get's.)

1. Make KTableKTableJoinMerger publie. (like KTableReduce, KTableAggregate.) 2. Add KTableKTableJoinMerger#of static methods: works as a workaround to the constructor. 3. Move [internalQueryableName, joinMerge, joinMergeProcessorParameters] variable in KTableImpl#doJoin into [NonMaterialized, Materialized]KTableKTableJoinNode.

…eJoinNode into KTableKTableJoinNode. 2. Move all materialization logic in KTableKTableJoinNode into KTable#doJoin.

…r#[builder, valueJoiner]

…KTableKTableJoinNode#queryableStoreName and use KTableKTableJoinMerger#getQueryableName instead.

…bleKTableJoinNodeBuilder

dongjinleekr · 2019-03-12T23:31:28Z

@guozhangwang Here is the fix. I omitted a file from the last commit. (+1. rebased onto the latest trunk.)

And also, I will separate the other operators from this issue in Jira. Is that okay?

cc/ @bbejeck

dongjinleekr · 2019-03-13T00:24:51Z

@guozhangwang Or, only separated PRs for this issue? @bbejeck

bbejeck · 2019-03-13T13:49:21Z

Java 8 failed with

org.apache.kafka.common.security.authenticator.SaslAuthenticatorTest.testValidSaslScramSha256
org.apache.kafka.common.security.authenticator.SaslAuthenticatorTest.testMultipleServerMechanisms
kafka.api.UserQuotaTest.testThrottledProducerConsumer

Java 11 passed

retest this please

bbejeck

This PR looks good to me, less just one minor comment.

bbejeck · 2019-03-13T14:37:30Z

Or, only separated PRs for this issue?

Fine with me to have multiple PRs on one Jira ticket, just make sure to have the Jira number in each PR title and in the PR comments state what parts you are working on.
\cc @guozhangwang

dongjinleekr · 2019-03-14T15:26:09Z

Here it is. OMG, I just learned one more thing about Java's final variable! Thanks @bbejeck :)

And about Issue management, I am +1 to separating the PRs only. In that case, two of the other PRs should be based on the other, since TableProcessorNode is used by all of filter, map, and transform. (In other words, one former PR will be the preliminary work for the other two PRs.)

I am now working on filter now. Let's roll! 😄

cc/ @guozhangwang

bbejeck

LGTM! Thanks @dongjinleekr

guozhangwang · 2019-03-15T01:41:26Z

LGTM!

…ilter) (#6453) This PR is a follow-up of #6174, which handles doFilter / doMapValues / doTransformValues methods. Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>

This PR is a follow-up of #6174 and #6453, which cleans up KTableImpl#doTransformValues method. Reviewers: Bill Bejeck <bbejeck@gmail.com>

* apache/trunk: MINOR: Retain public constructors of classes from public API (apache#6455) KAFKA-8118; Ensure ZK clients are closed in tests, fix verification (apache#6456) KAFKA-7813: JmxTool throws NPE when --object-name is omitted KAFKA-8114: Wait for SCRAM credential propagation in DelegationTokenEndToEndAuthorizationTest (apache#6452) KAFKA-8111; Set min and max versions for Metadata requests (apache#6451) KAFKA-7855: Kafka Streams Maven Archetype quickstart fails to compile out of the box (apache#6194) MINOR: Update code to not use deprecated methods (apache#6434) MINOR: Update Trogdor ConnectionStressWorker status at the end of execution (apache#6445) KAFKA-8091; Use commitSync to check connection failure in listener update test (apache#6450) KAFKA-7027: Add an overload build method in scala (apache#6373) MINOR: Fix typos in LogValidator (apache#6449) KAFKA-7502: Cleanup KTable materialization logic in a single place (apache#6174) KAFKA-7730; Limit number of active connections per listener in brokers (KIP-402) KAFKA-8091; Remove unsafe produce from dynamic listener update test (apache#6443) MINOR: Fix JavaDocs warnings (apache#6435) MINOR: Better messaging for invalid fetch response (apache#6427) MINOR: Use Java 8 lambdas in KStreamImplTest (apache#6430)

…pache#6174) This is a draft cleanup for KAFKA-7502. Here is the details: * Make KTableKTableJoinNode abstract, and define its child classes ([NonMaterialized,Materialized]KTableKTableJoinNode) instead: now, all materialization-related routines are separated into the other classes. * KTableKTableJoinNodeBuilder#build now instantiates [NonMaterialized,Materialized]KTableKTableJoinNode classes instead of KTableKTableJoinNode. Reviewers: Guozhang Wang <wangguoz@gmail.com>, Bill Bejeck <bbejeck@gmail.com>

…ilter) (apache#6453) This PR is a follow-up of apache#6174, which handles doFilter / doMapValues / doTransformValues methods. Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>

This PR is a follow-up of apache#6174 and apache#6453, which cleans up KTableImpl#doTransformValues method. Reviewers: Bill Bejeck <bbejeck@gmail.com>

mjsax added the streams label Jan 27, 2019

dongjinleekr force-pushed the feature/KAFKA-7502 branch from 79f0cc7 to c5d691c Compare February 22, 2019 13:46

guozhangwang reviewed Feb 22, 2019

View reviewed changes

dongjinleekr force-pushed the feature/KAFKA-7502 branch from c5d691c to e57ad22 Compare March 4, 2019 14:40

guozhangwang reviewed Mar 6, 2019

View reviewed changes

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

Comment thread ...ams/src/main/java/org/apache/kafka/streams/kstream/internals/graph/KTableKTableJoinNode.java Outdated

bbejeck reviewed Mar 6, 2019

View reviewed changes

dongjinleekr added 8 commits March 13, 2019 08:30

1. Separate Materialized-related logic into [NonMaterialized|Material…

a5ee071

…ized]KTableKTableJoinNode. 2. Add KTableKTableJoinNodeBuilde#builder.

Trivial: change method names following coding convention (i.e., remov…

30a699e

…e 'get's.)

1. Merge MaterializedKTableKTableJoinNode, NonMaterializedKTableKTabl…

ca57df5

…eJoinNode into KTableKTableJoinNode. 2. Move all materialization logic in KTableKTableJoinNode into KTable#doJoin.

Remove unused fields: KTableKTableJoinNode.KTableKTableJoinNodeBuilde…

f8a8d53

…r#[builder, valueJoiner]

1. Remove valueJoiner parameter from KTableKTableJoinNode. 2. Remove …

e39d8d7

…KTableKTableJoinNode#queryableStoreName and use KTableKTableJoinMerger#getQueryableName instead.

Remove InternalStreamsBuilder parameter from KTableKTableJoinNode#kTa…

c4afbff

…bleKTableJoinNodeBuilder

dongjinleekr force-pushed the feature/KAFKA-7502 branch from dfea944 to c4afbff Compare March 12, 2019 23:31

bbejeck reviewed Mar 13, 2019

View reviewed changes

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

Cleanup KTableKTableJoinNode building routine

0b278dc

bbejeck approved these changes Mar 14, 2019

View reviewed changes

guozhangwang merged commit 4ca8e40 into apache:trunk Mar 15, 2019

dongjinleekr mentioned this pull request Mar 16, 2019

KAFKA-7502: Cleanup KTable materialization logic in a single place (filter) #6453

Merged

3 tasks

This was referenced Mar 28, 2019

KAFKA-7502: Cleanup KTable materialization logic in a single place (doTransformValues) #6519

Merged

KAFKA-7502: Cleanup KTable materialization logic in a single place (doMapValues) #6520

Merged

bbejeck pushed a commit that referenced this pull request Mar 29, 2019

Cleanup KTableImpl#doTransformValues (#6519)

d10023e

This PR is a follow-up of #6174 and #6453, which cleans up KTableImpl#doTransformValues method. Reviewers: Bill Bejeck <bbejeck@gmail.com>

Conversation

dongjinleekr commented Jan 19, 2019

Committer Checklist (excluded from commit message)

Uh oh!

dongjinleekr commented Jan 19, 2019

Uh oh!

dongjinleekr commented Jan 30, 2019

Uh oh!

dongjinleekr commented Feb 22, 2019

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

dongjinleekr commented Mar 4, 2019

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guozhangwang commented Mar 6, 2019

Uh oh!

guozhangwang commented Mar 6, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bbejeck commented Mar 7, 2019

Uh oh!

bbejeck commented Mar 8, 2019

Uh oh!

bbejeck commented Mar 11, 2019

Uh oh!

dongjinleekr commented Mar 12, 2019

Uh oh!

guozhangwang commented Mar 12, 2019

Uh oh!

dongjinleekr commented Mar 12, 2019

Uh oh!

dongjinleekr commented Mar 13, 2019

Uh oh!

bbejeck commented Mar 13, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbejeck commented Mar 13, 2019

Uh oh!

dongjinleekr commented Mar 14, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants