Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Supply key-serde as well as value-serde in aggregate methods #172

Merged
merged 1 commit into from
Jul 30, 2019
Merged

Supply key-serde as well as value-serde in aggregate methods #172

merged 1 commit into from
Jul 30, 2019

Conversation

elzibubble
Copy link
Contributor

There's already a topic->materialized helper function, which I've
reused.

Signed-off-by: Alexis Lee alee@greshamtech.com

@elzibubble elzibubble requested a review from a team as a code owner July 24, 2019 14:36
@codecov
Copy link

codecov bot commented Jul 24, 2019

Codecov Report

Merging #172 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #172   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files          39       39           
  Lines        2364     2364           
  Branches      149      149           
=======================================
  Hits         1913     1913           
  Misses        302      302           
  Partials      149      149
Impacted Files Coverage Δ
src/jackdaw/streams/interop.clj 78.68% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775435c...a19ba0f. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 24, 2019 •
edited
Loading

Codecov Report

Merging #172 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #172   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files          39       39           
  Lines        2364     2364           
  Branches      149      149           
=======================================
  Hits         1913     1913           
  Misses        302      302           
  Partials      149      149
Impacted Files Coverage Δ
src/jackdaw/streams/interop.clj 78.68% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775435c...d4143f7. Read the comment docs.

@elzibubble
Copy link
Contributor Author

elzibubble commented Jul 24, 2019 •
edited
Loading

This doesn't seem to work... I'm wondering if Kafka also ignores it, for some reason.

Nope: https://github.com/apache/kafka/blob/2.2/streams/src/main/java/org/apache/kafka/streams/kstream/internals/KGroupedTableImpl.java#L100

@elzibubble
Copy link
Contributor Author

Welp, new tests pass so I guess Jackdaw is fine now, the bug I'm seeing in my private code must be something else. PR should be good to merge.

@elzibubble
Supply key-serde as well as value-serde in aggregate methods
d4143f7 
There's already a `topic->materialized` helper function, which I've
reused.

Signed-off-by: Alexis Lee <alee@greshamtech.com>
cddr
cddr approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@cddr cddr left a comment

Choose a reason for hiding this comment

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

LGTM. @99-not-out does this address any of the missing functionality you recently identified?

@cddr cddr merged commit 6dfc172 into FundingCircle:master Jul 30, 2019
@99-not-out
Copy link
Contributor

@cddr I will take a look - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cddr cddr cddr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elzibubble @99-not-out @cddr