Skip to content

KAFKA-6567: Remove KStreamWindowReducer#5922

Merged
guozhangwang merged 4 commits intoapache:trunkfrom
samuel-hawker:KAFKA-6567
Nov 20, 2018
Merged

KAFKA-6567: Remove KStreamWindowReducer#5922
guozhangwang merged 4 commits intoapache:trunkfrom
samuel-hawker:KAFKA-6567

Conversation

@samuel-hawker
Copy link
Copy Markdown
Contributor

@samuel-hawker samuel-hawker commented Nov 17, 2018

This pull request removes the final reference to KStreamWindowReducer and replaces it with KStreamWindowAggregate

Signed-off-by: Samuel Hawker sam.b.hawker@gmail.com

contribution is my original work and that I license the work to the project under the project's open source license.

Committer Checklist (excluded from commit message)

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

Signed-off-by: Samuel Hawker <samuel.hawker@ibm.com>
@samuel-hawker samuel-hawker changed the title fix: Remove KStreamWindowReducer KAFKA-6567 Remove KStreamWindowReducer Nov 17, 2018
@samuel-hawker samuel-hawker changed the title KAFKA-6567 Remove KStreamWindowReducer KAFKA-6567: Remove KStreamWindowReducer Nov 17, 2018
@mjsax mjsax added the streams label Nov 19, 2018
Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

Just one nit. Overall LGTM.

Objects.requireNonNull(materialized, "materialized can't be null");

final MaterializedInternal<K, V, WindowStore<Bytes, byte[]>> materializedInternal = new MaterializedInternal<>(materialized);
final Aggregator<K, V, V> reduceAggregator = aggregatorForReducer(reducer);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: I would inline this method. It's a single liner and only used once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have inlined it, good catch.

Signed-off-by: Samuel Hawker <sam.b.hawker@gmail.com>
@samuel-hawker
Copy link
Copy Markdown
Contributor Author

The build failed, not sure why, they worked locally and I only moved the function call inline

@samuel-hawker
Copy link
Copy Markdown
Contributor Author

retest this please

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!

@guozhangwang guozhangwang merged commit 8dc4d0e into apache:trunk Nov 20, 2018
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Nov 20, 2018

Thanks for the PR @samuel-hawker and congrats to your first contribution to Kafka! 🎉

@samuel-hawker samuel-hawker deleted the KAFKA-6567 branch November 20, 2018 09:32
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
This pull request removes the final reference to KStreamWindowReducer and replaces it with KStreamWindowAggregate

Signed-off-by: Samuel Hawker sam.b.hawker@gmail.com

contribution is my original work and that I license the work to the project under the project's open source license.

Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants