Skip to content

KAFKA-9892: Producer state snapshot needs to be forced to disk#9621

Merged
hachikuji merged 2 commits into
apache:trunkfrom
bristy:trunk
Dec 9, 2020
Merged

KAFKA-9892: Producer state snapshot needs to be forced to disk#9621
hachikuji merged 2 commits into
apache:trunkfrom
bristy:trunk

Conversation

@bristy

@bristy bristy commented Nov 19, 2020

Copy link
Copy Markdown
Contributor

FileChannel.close() does not guarantee modified buffer would be written on the file system. We are changing it with force() semantics to enforce file buffer and metadata written to filesystem ( FileChannel.force(true) updates buffer and metadata).

*Summary of testing strategy (including rationale)
I have run unittests after making the changes.

@junrao junrao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bristy : Thanks for the PR. Left a comment below.

Comment thread core/src/main/scala/kafka/log/ProducerStateManager.scala Outdated
@hachikuji

Copy link
Copy Markdown
Contributor

@bristy Thanks for the patch. I'd suggest changing to this:

    try {
      fileChannel.write(buffer)
      fileChannel.force(true)
    } finally {
      fileChannel.close()
    }

If you don't have time to get back to this, I can push a commit.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

@junrao junrao left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bristy : Thanks for the updated PR. LGTM

@hachikuji hachikuji merged commit fa93982 into apache:trunk Dec 9, 2020
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.

3 participants