KAFKA-7492 : Updated javadocs for aggregate and reduce methods returning null behavior. by asutosh936 · Pull Request #6285 · apache/kafka

asutosh936 · 2019-02-18T19:04:44Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

This is update to the existing javadocs for KGroupedStream class. More details are available at
https://stackoverflow.com/questions/52692202/what-happens-if-the-aggregator-of-a-kgroupedstream-returns-null

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

…ing null behavior.

asutosh936 · 2019-02-18T23:07:42Z

@mjsax - Have take a stab updating the documentation. Kindly review.

mjsax

Thanks for the PR @asutosh936.

Call for second review @guozhangwang @bbejeck @vvcephei @ableegoldman

mjsax · 2019-02-19T23:33:23Z

     * @param reducer   a {@link Reducer} that computes a new aggregate result. Cannot be {@code null}.
     * @return a {@link KTable} that contains "update" records with unmodified keys, and values that represent the
-     * latest (rolling) aggregate for each key
+     * latest (rolling) aggregate for each key. Incase {@link KTable} returns null the following message will be


typo: In case

nit: null -> {@code null}

Similar comments below.

What do you mean by KTable returns null ? Do you mean Reducer#apply(..) returns null?

How about this: If the reduce function returns null, it is then interpreted as deletion for the key, and future messages of the same key coming from upstream operators will be handled as newly initialized value.

Should we mention the "problem" with out-of-order data for this case? Should we ever recommend to not return null ?

We had a discussion at some point to actually disallow returning null because a "delete" is not a valid aggregation result.

It's a nuanced topic. It would be good to clarify if we can find a good way to say it.

The operator itself never processes data out of order. It always respects the order defined by the topic. But upstream repartitions don't provide any guarantees about the order they write to the topic. This is how data can logically become out of order, even if users take care to populate their input topics strictly ordered, which might in turn violate some expectations with nulls in the picture.

I'm concerned that if we start saying, "Kafka Streams processes data out of order", with no context, it'll create FUD. We should be up front about the limits of the system, but it doesn't benefit anyone to create the impression that we don't guarantee things that we actually do guarantee.

In other words, I'm recommending that we specifically say something like "Producing deletes from your aggregations may cause unexpected results when processing dis-ordered data. Streams always processes data in the order it appears in the topic. If the topic is populated out of order, you may have late arriving records, which can cause records to become unexpectedly re-created after they have been deleted. Out-of-order data can be a problem for non-deleting aggregation functions as well, but it's especially surprising with aggregations that produce deletes."

:/ ... you see what I mean by saying that it's a nuanced topic.

I agree it's nuanced. (Don't agree with all you say above -- omit details for now)

Seems we get out-of-scope for this PR... Maybe JavaDocs are not the right place to document this anyway?

I'd agree that javadocs are not the best place to document out-of-ordering here.

vvcephei · 2019-02-20T16:41:24Z

     * @return a {@link KTable} that contains "update" records with unmodified keys, and values that represent the
-     * latest (rolling) aggregate for each key
+     * latest (rolling) aggregate for each key. Incase {@link KTable} returns null the following message will be
+     * processed as if it was the first message.


nit: was -> were (also below)

…ing null behavior. Implemented review comments

asutosh936 · 2019-02-21T02:16:30Z

Updated comment as per @guozhangwang.

bbejeck

Thanks for the contribution @asutosh936 LGTM

guozhangwang · 2019-02-22T16:59:55Z

LGTM!

bbejeck · 2019-02-22T17:07:47Z

merged #6285 to trunk

mjsax · 2019-02-23T01:02:36Z

@asutosh936 What is your Jira Account so we can assign the ticket to you?

asutosh936 · 2019-02-23T01:20:57Z

@mjsax - Have given comment on Jira. I can assign the ticket to myself if needed.

* AK/trunk: (36 commits) KAFKA-7962: Avoid NPE for StickyAssignor (apache#6308) Address flakiness of CustomQuotaCallbackTest#testCustomQuotaCallback (apache#6330) KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches (apache#6327) MINOR: fix parameter naming (apache#6316) KAFKA-7956 In ShutdownableThread, immediately complete the shutdown if the thread has not been started (apache#6218) MINOR: Refactor replica log dir fetching for improved logging (apache#6313) [TRIVIAL] Remove unused StreamsGraphNode#repartitionRequired (apache#6227) MINOR: Increase produce timeout to 120 seconds (apache#6326) KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store (apache#6293) MINOR: Fix line break issue in upgrade notes (apache#6320) KAFKA-7972: Use automatic RPC generation in SaslHandshake MINOR: Enable capture of full stack trace in StreamTask#process (apache#6310) KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest (apache#6312) KAFKA-7937: Fix Flaky Test ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup (apache#6311) MINOR: Update docs to say 2.2 (apache#6315) KAFKA-7672 : force write checkpoint during StreamTask #suspend (apache#6115) KAFKA-7961; Ignore assignment for un-subscribed partitions (apache#6304) KAFKA-7672: Restoring tasks need to be closed upon task suspension (apache#6113) KAFKA-7864; validate partitions are 0-based (apache#6246) KAFKA-7492 : Updated javadocs for aggregate and reduce methods returning null behavior. (apache#6285) ...

…ing null behavior. (apache#6285) This is an update to the existing javadocs for KGroupedStream class. Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

KAFKA-7492 : Updated javadocs for aggregate and reduce methods return…

ede2823

…ing null behavior.

mjsax reviewed Feb 19, 2019

View reviewed changes

vvcephei reviewed Feb 20, 2019

View reviewed changes

KAFKA-7492 : Updated javadocs for aggregate and reduce methods return…

fcb364b

…ing null behavior. Implemented review comments

bbejeck added the streams label Feb 22, 2019

bbejeck approved these changes Feb 22, 2019

View reviewed changes

bbejeck merged commit 93ba962 into apache:trunk Feb 22, 2019

Conversation

asutosh936 commented Feb 18, 2019

Committer Checklist (excluded from commit message)

Uh oh!

asutosh936 commented Feb 18, 2019

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asutosh936 commented Feb 21, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Feb 22, 2019

Uh oh!

bbejeck commented Feb 22, 2019

Uh oh!

mjsax commented Feb 23, 2019

Uh oh!

asutosh936 commented Feb 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants