Skip to content

MINOR: Align KTableAgg and KTableReduce#6712

Merged
mjsax merged 2 commits intoapache:trunkfrom
mjsax:minor-unify-table-agg-reduce
May 11, 2019
Merged

MINOR: Align KTableAgg and KTableReduce#6712
mjsax merged 2 commits intoapache:trunkfrom
mjsax:minor-unify-table-agg-reduce

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented May 10, 2019 •
edited
Loading

This PR aligns the logic of KTableAgg and KTableReduce and makes the code more readable. It also fixes some bugs in both implementations:

  • 'remove' should be done before 'add'
  • 'remove' should only be done if an old value exists in the store
  • init() should not be called for 'remove' case if old value does not exist in store

@mjsax mjsax added the streams label May 10, 2019
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 10, 2019

While working on the "use TS in DSL" PR, I encountered that KTableAgg and KTableReduce apply different logic. I think we should also back port this to 2.2, 2.1, and 2.0. (Cut off point being a major release.) For trunk I will also do a follow up PR, that actually "removed" reduce (for KTable and KStream case) and expressed reduce as aggregation.

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman @abbccdda @cadonna

vvcephei
vvcephei approved these changes May 10, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks!

bbejeck
bbejeck approved these changes May 10, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks @mjsax LGTM

jeffkbkim
jeffkbkim reviewed May 10, 2019
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableReduce.java Outdated
intermediateAgg = oldAgg;
}

// than try to add the new value
Copy link
Copy Markdown
Contributor

@jeffkbkim jeffkbkim May 10, 2019

Choose a reason for hiding this comment

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

than -> then?

guozhangwang
guozhangwang approved these changes May 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM modulo @jeffkbkim 's comment. Thanks for the great catch!

Please feel free to merge and also cherry-pick to old branches.

@mjsax mjsax merged commit 7c20257 into apache:trunk May 11, 2019
@mjsax mjsax deleted the minor-unify-table-agg-reduce branch May 11, 2019 09:55
mjsax added a commit that referenced this pull request May 11, 2019
@mjsax
MINOR: Align KTableAgg and KTableReduce (#6712)
4ed5efe 
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Jeff Kim <kimkb2011@gmail.com>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request May 11, 2019
@mjsax
MINOR: Align KTableAgg and KTableReduce (#6712)
5a632f1 
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Jeff Kim <kimkb2011@gmail.com>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request May 11, 2019
@mjsax
MINOR: Align KTableAgg and KTableReduce (#6712)
f81bad2 
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Jeff Kim <kimkb2011@gmail.com>, Guozhang Wang <guozhang@confluent.io>
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented May 11, 2019

Merged to trunk and cherry-picked to 2.2, 2.1, and 2.0 branches.

omkreddy added a commit to confluentinc/kafka that referenced this pull request May 13, 2019
@omkreddy
Merge remote-tracking branch 'AK_REPO/trunk' into sync-upstream-chang…
8421da6 
…es-14-May

* AK_REPO/trunk: (24 commits)
  KAFKA-7321: Add a Maximum Log Compaction Lag (KIP-354) (apache#6009)
  KAFKA-8335; Clean empty batches when sequence numbers are reused (apache#6715)
  KAFKA-6455: Session Aggregation should use window-end-time as record timestamp (apache#6645)
  KAFKA-6521: Use timestamped stores for KTables (apache#6667)
  [MINOR] Consolidate in-memory/rocksdb unit tests for window & session store (apache#6677)
  MINOR: Include StickyAssignor in system tests (apache#5223)
  KAFKA-7633: Allow Kafka Connect to access internal topics without cluster ACLs (apache#5918)
  MINOR: Align KTableAgg and KTableReduce (apache#6712)
  MINOR: Fix code section formatting in TROGDOR.md (apache#6720)
  MINOR: Remove unnecessary OptionParser#accepts method call from PreferredReplicaLeaderElectionCommand (apache#6710)
  KAFKA-8352 : Fix Connect System test failure 404 Not Found (apache#6713)
  KAFKA-8348: Fix KafkaStreams JavaDocs (apache#6707)
  MINOR: Add missing option for running vagrant-up.sh with AWS to vagrant/README.md
  KAFKA-8344; Fix vagrant-up.sh to work with AWS properly
  MINOR: docs typo in '--zookeeper myhost:2181--execute'
  MINOR: Remove header and key/value converter config value logging (apache#6660)
  KAFKA-8231: Expansion of ConnectClusterState interface (apache#6584)
  KAFKA-8324: Add close() method to RocksDBConfigSetter (apache#6697)
  KAFKA-6789; Handle retriable group errors in AdminClient API (apache#5578)
  KAFKA-8332: Refactor ImplicitLinkedHashSet to avoid losing ordering when converting to Scala
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mjsax @pengxiaolong
MINOR: Align KTableAgg and KTableReduce (apache#6712)
1b71665 
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Jeff Kim <kimkb2011@gmail.com>, Guozhang Wang <guozhang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeffkbkim jeffkbkim jeffkbkim left review comments

@bbejeck bbejeck bbejeck approved these changes

@vvcephei vvcephei vvcephei approved these changes

@guozhangwang guozhangwang guozhangwang approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mjsax @bbejeck @vvcephei @guozhangwang @jeffkbkim