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.

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

MINOR: improve Session expiration notice #6618

Merged
merged 4 commits into from
Apr 27, 2019
Merged

MINOR: improve Session expiration notice #6618

merged 4 commits into from
Apr 27, 2019

Conversation

vvcephei
Copy link
Contributor

The log for expired session windows only shows the window end time and expiration time, not the current stream time, which makes it not obvious why the window is expired, unless you are intimately familiar with stream-time calculations. Explicitly logging the current stream time should help.

Committer Checklist (excluded from commit message)

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

@vvcephei
Copy link
Contributor Author

@cadonna @mjsax , do you mind taking a quick look at this logging improvement? Thanks!

@mjsax mjsax added the streams label Apr 22, 2019
Copy link
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.

LGTM.

@vvcephei
Copy link
Contributor Author

Thanks! Should we cherry-pick this to 2.2 and 2.1 as well?

@mjsax
Copy link
Member

mjsax commented Apr 23, 2019

@vvcephei Test failure seems related:

java.lang.AssertionError: 
Expected: a collection containing "Skipping record for expired window. key=[A] topic=[topic] partition=[-3] offset=[-2] timestamp=[0] window=[0,0) expiration=[10]"
     but: mismatches were: [was "Skipping record for expired window. key=[A] topic=[topic] partition=[-3] offset=[-2] timestamp=[0] window=[0,0) expiration=[10] streamTime=[20]", was "Skipping record for expired window. key=[A] topic=[topic] partition=[-3] offset=[-2] timestamp=[1] window=[1,1) expiration=[10] streamTime=[20]"]
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.apache.kafka.streams.kstream.internals.KStreamSessionWindowAggregateProcessorTest.shouldLogAndMeterWhenSkippingLateRecord(KStreamSessionWindowAggregateProcessorTest.java:383)

@vvcephei
Copy link
Contributor Author

Oops! Thanks for capturing that.

@ableegoldman
Copy link
Member

@vvcephei Would this be helpful for time-based windows as well?

@vvcephei
Copy link
Contributor Author

@ableegoldman , yes, that's a good thought. I'll add it.

@vvcephei
Copy link
Contributor Author

Ok, all comments are addressed so far... Do you mind taking another look @mjsax and @ableegoldman ?

Copy link
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

LGTM

@mjsax
Copy link
Member

mjsax commented Apr 25, 2019

@vvcephei Test failures seem to be related?

@vvcephei
Copy link
Contributor Author

Weird, I couldn't reproduce the test failures.

Retest this, please.

@mjsax
Copy link
Member

mjsax commented Apr 25, 2019

@vvcephei Failed again -- there seems to be something... no clue what. Maybe try to rebase to trunk ?

@vvcephei
Copy link
Contributor Author

Ok, got it. The tests may log extra messages that didn't get logged on my machine, so we can't use equality for validating the log results.

@mjsax mjsax merged commit 39400c4 into apache:trunk Apr 27, 2019
@vvcephei vvcephei deleted the MINOR-improve-session-expiration-notice branch April 29, 2019 14:32
@vvcephei
Copy link
Contributor Author

Thanks, @mjsax ! Can we also cherry-pick this to 2.2 and 2.1 ? (there is only one minor conflict in the 2.1 branch)

dhruvilshah3 added a commit to confluentinc/kafka that referenced this pull request Apr 29, 2019
* ak/trunk: (42 commits)
  KAFKA-8134: `linger.ms` must be a long
  KAFKA-7779; Avoid unnecessary loop iteration in leastLoadedNode (apache#6081)
  MINOR: Update Gradle to 5.4.1 and update its plugins  (apache#6436)
  MINOR: improve Session expiration notice (apache#6618)
  KAFKA-8029: In memory session store (apache#6525)
  MINOR: In-memory stores cleanup (apache#6595)
  KAFKA-7862 & KIP-345 part-one: Add static membership logic to JoinGroup protocol (apache#6177)
  KAFKA-8254: Pass Changelog as Topic in Suppress Serdes (apache#6602)
  KAFKA-7903: automatically generate OffsetCommitRequest (apache#6583)
  KAFKA-8291 : System test fix (apache#6637)
  MINOR: Do not log retriable offset commit exceptions as errors (apache#5904)
  MINOR: Fix log message error of loadTransactionMetadata (apache#6571)
  MINOR: Fix 404 security features links (apache#6634)
  MINOR: Remove an unnecessary character from broker's startup log
  MINOR: Make LogCleaner.shouldRetainRecord more readable (apache#6590)
  MINOR: Remove implicit return statement (apache#6629)
  KAFKA-8237; Untangle TopicDeleteManager and add test cases (apache#6588)
  KAFKA-8227 DOCS Fixed missing links duality of streams tables (apache#6625)
  MINOR: reformat settings.gradle to be more readable (apache#6621)
  MINOR: Correct RestServerTest formatting
  ...

 Conflicts:
	build.gradle
	settings.gradle
mjsax pushed a commit that referenced this pull request Apr 29, 2019
Reviewers: Matthias J. Sax <[email protected]>, A. Sophie Blee-Goldman <[email protected]>
mjsax pushed a commit that referenced this pull request Apr 30, 2019
Reviewers: Matthias J. Sax <[email protected]>, A. Sophie Blee-Goldman <[email protected]>
@mjsax
Copy link
Member

mjsax commented Apr 30, 2019

Cherry-picked to 2.2 and 2.1 branches.

@vvcephei
Copy link
Contributor Author

vvcephei commented May 2, 2019

Thanks so much!

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
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