Skip to content

KAFKA-12536: Add Instant-based methods to ReadOnlySessionStore#10390

Merged
vvcephei merged 6 commits intoapache:trunkfrom
jeqo:kip-666
May 7, 2021
Merged

KAFKA-12536: Add Instant-based methods to ReadOnlySessionStore#10390
vvcephei merged 6 commits intoapache:trunkfrom
jeqo:kip-666

Conversation

@jeqo
Copy link
Copy Markdown
Contributor

@jeqo jeqo commented Mar 23, 2021

KIP-666 implementation.

Committer Checklist (excluded from commit message)

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

@mjsax mjsax added the streams label Mar 27, 2021
@jeqo jeqo mentioned this pull request May 7, 2021
Closed
3 tasks
vvcephei
vvcephei approved these changes May 7, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM! Thanks, @jeqo !

@vvcephei vvcephei merged commit 8f8f914 into apache:trunk May 7, 2021
@jeqo jeqo deleted the kip-666 branch May 7, 2021 18:25
@dotjdk
Copy link
Copy Markdown

dotjdk commented Mar 28, 2022

The parameter name changes for fetchSession in this commit introduces some potential confusion. Looks like a copy/paste mistake from the findSessions methods. fetchSession parameters should be called startTime and endTime as they where in previous releases.

@jeqo jeqo mentioned this pull request Apr 5, 2022
Merged
3 tasks
@jeqo
Copy link
Copy Markdown
Contributor Author

jeqo commented Apr 5, 2022

Thanks @dotjdk !
Do you mean

kafka/streams/src/main/java/org/apache/kafka/streams/state/SessionStore.java

Line 92 in 8f8f914

default AGG fetchSession(final K key, final Instant earliestSessionEndTime, final Instant latestSessionStartTime) {
specifically? If so, I agree. Check if this PR fixes the issue: to open it for review: #11999.

@dotjdk
Copy link
Copy Markdown

dotjdk commented Apr 6, 2022

Yes. I have seen the PR, and that fixes the misleading parameter names 👍

vvcephei pushed a commit that referenced this pull request Apr 11, 2022
@jeqo
MINOR: Fix SessionStore#fetchSession parameter names (#11999)
0d518aa 
Fixes a small copy/paste error from #10390 that changed the parameter names
for fetchSession from the singular session form (eg `startTime`) to the range
form (eg `earliestSessionStartTime`).

Reviewers: John Roesler <vvcephei@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vvcephei vvcephei vvcephei approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jeqo @dotjdk @vvcephei @mjsax