Skip to content

MINOR: Make Log.recordVersion private and other small cleanups#9731

Merged
chia7712 merged 2 commits into
apache:trunkfrom
kowshik:MINOR_refactor_to_use_recordVersion
Dec 14, 2020
Merged

MINOR: Make Log.recordVersion private and other small cleanups#9731
chia7712 merged 2 commits into
apache:trunkfrom
kowshik:MINOR_refactor_to_use_recordVersion

Conversation

@kowshik

@kowshik kowshik commented Dec 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Couple small cleanups in this PR:

  • I've converted Log.recordVersion to be private, since it is used only inside Log class. I've also changed one of the call sites to reuse the same method.
  • In a few classes I've removed a redundant default parameter fileAlreadyExists = false in a call to LogSegment.open.

Tests:
Relying on existing unit & integration tests.

@kowshik

kowshik commented Dec 11, 2020

Copy link
Copy Markdown
Contributor Author

cc @junrao @ijuma @chia7712 for review

@kowshik kowshik force-pushed the MINOR_refactor_to_use_recordVersion branch from 4d7ada2 to 496554b Compare December 11, 2020 03:24
chia7712
chia7712 approved these changes Dec 11, 2020
View reviewed changes

@chia7712 chia7712 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.

@kowshik thanks for this patch. some small comments are left. overall +1

Comment thread core/src/main/scala/kafka/log/Log.scala
Comment thread core/src/main/scala/kafka/log/Log.scala
@kowshik

kowshik commented Dec 11, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the review @chia7712 ! I've addressed the comments in 2d2b4a2.

@kowshik kowshik changed the title MINOR: Make Log.recordVersion private and refactor to use the same MINOR: Make Log.recordVersion private and refactor to reuse the same Dec 11, 2020
@kowshik kowshik changed the title MINOR: Make Log.recordVersion private and refactor to reuse the same MINOR: Make Log.recordVersion private and other small cleanups Dec 11, 2020
@kowshik kowshik requested a review from chia7712 December 11, 2020 17:59
@kowshik

kowshik commented Dec 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@chia7712 if the change looks good to you, please could you help merge it? :)

chia7712
chia7712 approved these changes Dec 12, 2020
View reviewed changes

@chia7712 chia7712 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.

@kowshik LGTM

will commit it tomorrow if there is no objection.

@chia7712 chia7712 merged commit 4401f52 into apache:trunk Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kowshik @chia7712