Skip to content

MINOR: Avoid double null check in KStream::transform()#6429

Merged
mjsax merged 1 commit intoapache:trunkfrom
cadonna:double_null_check
Mar 12, 2019
Merged

MINOR: Avoid double null check in KStream::transform()#6429
mjsax merged 1 commit intoapache:trunkfrom
cadonna:double_null_check

Conversation

@cadonna
Copy link
Copy Markdown
Member

@cadonna cadonna commented Mar 11, 2019

A call to KStream::transform() checked twice the parameter
transformerSupplier for null. The double null check occurred
because KStream::transform() is expressed in terms of
KStream::flatTransform() and both methods performed the null check.

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.

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)

A call to KStream::transform() checked twice the parameter
transformerSupplier for null. The double null check occurred
because KStream::transform() is expressed in terms of
KStream::flatTransform() and both methods performed the null check.
@bbejeck bbejeck requested review from guozhangwang and mjsax March 11, 2019 21:54
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Mar 11, 2019

\cc @vvcephei and @ableegoldman for reviews

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 12, 2019

I see the intent, but the disadvantage is, that we get deeper stack traces, ie, the user will see doFlatTransform() throwing a NPE, however, user calls either transform() or flatTransform(). I personally prefer the current code, because it's give better error messages to users IMHO. It's subtle of course, especially for this case, as the stack trace will be only one level deeper. I still think, it's easier for users to see

NPE
  at ...transform()
...

instead of

NPE
  at ...doTransform()
  at ...transform()
...

Also, the PR only allows to share one line of code (so the benefits of the rewrite is small).

Thoughts?

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Mar 12, 2019

@mjsax thank you for your comment.

The NPEs are still thrown by transform and flatTransform. So the stack trace will not get deeper. Furthermore, the PR removes one line from the shared code rather than adding one, namely the null check in flatTransform (see line 452 in the original code).

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.

Ack.

Guess -- maybe it was too late to read a diff...

@mjsax mjsax merged commit 2aca624 into apache:trunk Mar 12, 2019
jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
* warn-apache-kafka/trunk: (41 commits)
  MINOR: Avoid double null check in KStream#transform() (apache#6429)
  KAFKA-7944: Improve Suppress test coverage (apache#6382)
  KAFKA-3522: add missing guards for TimestampedXxxStore (apache#6356)
  MINOR: Change Trogdor agent's cleanup executor to a cached thread pool (apache#6309)
  KAFKA-7976; Update config before notifying controller of unclean leader update (apache#6426)
  KAFKA-7801: TopicCommand should not be able to alter transaction topic partition count
  KAFKA-8091; Wait for processor shutdown before testing removed listeners (apache#6425)
  MINOR: Update delete topics zk path in assertion error messages
  KAFKA-7939: Fix timing issue in KafkaAdminClientTest.testCreateTopicsRetryBackoff
  KAFKA-7922: Return authorized operations in Metadata request response (KIP-430 Part-2)
  MINOR: Print usage when parse fails during console producer
  MINOR: fix Scala compiler warning (apache#6417)
  KAFKA-7288; Fix check in SelectorTest to wait for no buffered bytes (apache#6415)
  KAFKA-8065: restore original input record timestamp in forward() (apache#6393)
  MINOR: cleanup deprectaion annotations (apache#6290)
  KAFKA-3522: Add TimestampedWindowStore builder/runtime classes (apache#6173)
  KAFKA-8069; Fix early expiration of offsets due to invalid loading of expire timestamp (apache#6401)
  KAFKA-8070: Increase consumer startup timeout in system tests (apache#6405)
  KAFKA-8040: Streams handle initTransactions timeout (apache#6372)
  KAFKA-7980 - Fix timing issue in SocketServerTest.testConnectionRateLimit (apache#6391)
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>
@cadonna cadonna deleted the double_null_check branch October 21, 2019 10:53
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.

4 participants