Skip to content

KAFKA-8731: InMemorySessionStore throws NullPointerException on startup#7132

Merged
bbejeck merged 2 commits into
apache:trunkfrom
ableegoldman:fix-IMStore-remove-bug
Jul 31, 2019
Merged

KAFKA-8731: InMemorySessionStore throws NullPointerException on startup#7132
bbejeck merged 2 commits into
apache:trunkfrom
ableegoldman:fix-IMStore-remove-bug

Conversation

@ableegoldman

@ableegoldman ableegoldman commented Jul 29, 2019

Copy link
Copy Markdown
Member

Should be cherry-picked to 2.3

@ableegoldman

Copy link
Copy Markdown
Member Author

@Override
public void remove(final Windowed<Bytes> sessionKey) {
final ConcurrentNavigableMap<Bytes, ConcurrentNavigableMap<Long, byte[]>> keyMap = endTimeMap.get(sessionKey.window().end());
if (keyMap == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How could keyMap be null, ie, why would we call remove()for a non-existing session (at least in the DSL)?

Not saying that the fix does not make sense, I am just wondering if we need an additional fix for the DSL?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well. Thinking about it twice, during restore maybe? Tombstone are preserved longer in the changelog, hence, for a deleted session we only have the tombstone. On restore, we would never see and "insert" but only the delete.

Maybe worth double checking in the DSL code anyway.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I assume a tombstone was being restored, where the corresponding put was already cleaned up from the changelog?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Answered yourself quicker than me. But ok, I'll look around regardless

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm fairly confident the only affected case is on the restore path when the changelog contains a tombstone that outlived its corresponding "put" -- during normal processing, if the delete arrives after the key has already been cleaned up it will be dropped as a late record anyways. Assuming we aren't accidentally sending double tombstones anywhere.

@mjsax

mjsax commented Jul 30, 2019

Copy link
Copy Markdown
Member

This bug basically make the in-memory store unusable in practice. We should have a proper ticket for tracking. It's not just a HOTFIX.

@jonathanpdx

Copy link
Copy Markdown

Here's a Jira ticket:

https://issues.apache.org/jira/browse/KAFKA-8731

@ableegoldman

Copy link
Copy Markdown
Member Author

Thanks for the ticket!

Java8 and Java 11/2.12 passed, Java 11/2.13 failed with known flaky test org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testStartTwoConnectors

retest this, please

@ableegoldman ableegoldman changed the title HOTFIX: InMemorySessionStore throws NPE removing non-existent key KAFKA-8731: InMemorySessionStore throws NullPointerException on startup Jul 30, 2019

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the quick fix! LGTM.

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix @ableegoldman LGTM

@mjsax

mjsax commented Jul 31, 2019

Copy link
Copy Markdown
Member

RebalanceSourceConnectorsIntegrationTest failed for Java11 / 2.12 and Java8.

Java 11 / 2.13 passed.

Retest this please.

@bbejeck bbejeck merged commit a028f59 into apache:trunk Jul 31, 2019
@bbejeck

bbejeck commented Jul 31, 2019

Copy link
Copy Markdown
Member

Merged #7132 into trunk

bbejeck pushed a commit that referenced this pull request Jul 31, 2019
…up (#7132)

Reviewers:  Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
@bbejeck

bbejeck commented Jul 31, 2019

Copy link
Copy Markdown
Member

cherry-picked to 2.3

ijuma added a commit to confluentinc/kafka that referenced this pull request Aug 4, 2019
* apache-github/2.3:
  MINOR: Avoid dividing by zero (apache#7143)
  KAFKA-8731: InMemorySessionStore throws NullPointerException on startup (apache#7132)
  KAFKA-8715; Fix buggy reliance on state timestamp in static member.id generation (apache#7116)
  KAFKA-8678; Fix leave group protocol bug in throttling and error response (apache#7101)
xiowu0 pushed a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
…llPointerException on startup (apache#7132)

TICKET = KAFKA-8731
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [f86f7e2]
ORIGINAL_DESCRIPTION =

Reviewers:  Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
(cherry picked from commit f86f7e2)
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