Skip to content

KAFKA-12815: Update JavaDocs of ValueTransformerWithKey #10731

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.

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

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented May 19, 2021

Follow up to #10720

Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@mjsax , thanks for the PR. Left a comment. Thanks.

* <p>
* Note that if a {@code ValueTransformerWithKey} is used in a {@link KTable#transformValues(ValueTransformerWithKeySupplier, String...)}
* (or any other overload of {@code KTable#transformValues(...)}) operation,
* that the provided {@link ProcessorContext} from {@link #init(ProcessorContext)}
Copy link
Member

Choose a reason for hiding this comment

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

is this a typo?
if a 'xxx' is used in a 'yyy' operation, that the provided 'zzz' from 'aaa' doesn't guarantee....

Maybe replace that into then?

Copy link
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, @mjsax ! What do you think about actually specifying what happens when these things are "not available"?

Comment on lines 90 to 91
* does not guarantee that all context information will be available when {@code transform()}
* is executed.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might as well formally specify which things may be missing. Otherwise, it's not clear what users should do to guard their code.

Suggested change
* does not guarantee that all context information will be available when {@code transform()}
* is executed.
* does not guarantee that all record context information will be available when
* {@code transform()} is executed. For example, {@code transform()} may be
* invoked as a result of upstream punctuations or other out-of-band operations,
* in which case there will be no headers, topic name, timestamp, or offset available.
* When those properties are absent, the following placeholder values will
* be filled in: empty headers, `null` topic name, `-1` timestamp, and `-1` offset.
* Implementations of this interface should guard against this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, for KTable#transformValues those things don't apply because there is no out-of-band transformation upstream.

It's the DSL, not the PAPI. It might might sense to add this information in general, but seems to be out-of-scope for this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

All I was trying to do was characterize why the context information might be missing. If there are no out-of-band operations, then there should never be missing context.

// we move to `RecordMetadata` than would be `null` for this case and thus
// we won't have the partition information, so it's better to now provide it
// here either, to not introduce a regression later on
-1,
Copy link
Member Author

Choose a reason for hiding this comment

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

@vvcephei I think it's better to change this...

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, @mjsax .

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, thanks @mjsax to bring to our attention.

* then the provided {@link ProcessorContext} from {@link #init(ProcessorContext)}
* does not guarantee that all context information will be available when {@code transform()}
* is executed, as it might be executed "out-of-band" due to some internal optimizations
* applied by the Kafka Streams DSL.
Copy link
Member Author

Choose a reason for hiding this comment

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

@vvcephei Updated the text a little bit.

Don't think we should talk about punctuations because it does not apply for the "ValueGetter" optimization case that we fixed. Did a follow up PR instead: #10810

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@mjsax mjsax merged commit b5f7ce8 into apache:trunk Jun 8, 2021
@mjsax mjsax deleted the fix-timestamp branch June 8, 2021 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants