Skip to content

MINOR: Fix some java docs of ReplicaStateMachine#8552

Merged
chia7712 merged 1 commit into
apache:trunkfrom
dengziming:MINOR-java-docs
Dec 10, 2020
Merged

MINOR: Fix some java docs of ReplicaStateMachine#8552
chia7712 merged 1 commit into
apache:trunkfrom
dengziming:MINOR-java-docs

Conversation

@dengziming

@dengziming dengziming commented Apr 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Modify the java docs of ReplicaStateMachine, the java docs is not in consistent with OfflineReplica.validPreviousStates and OnlineReplica.validPreviousStates

Move the repeated if(traceEnabled) before logSuccessfulTransition method to the method

Committer Checklist (excluded from commit message)

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

@dengziming dengziming changed the title [MINOR]: Fix some java docs [MINOR]: Fix some java docs and move repeated if before the same method to the method Apr 25, 2020
@dengziming dengziming changed the title [MINOR]: Fix some java docs and move repeated if before the same method to the method [MINOR]: Fix some java docs and move repeated if to the method Apr 25, 2020
@dengziming dengziming changed the title [MINOR]: Fix some java docs and move repeated if to the method [MINOR]: Fix some java docs and move repeated if conditioni to the method May 29, 2020
hachikuji
hachikuji reviewed Jun 7, 2020
View reviewed changes

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

Thanks, left a minor comment.

Comment thread core/src/main/scala/kafka/controller/ReplicaStateMachine.scala Outdated
@dengziming dengziming changed the title [MINOR]: Fix some java docs and move repeated if conditioni to the method MINOR: Fix some java docs and move repeated if condition to the method Oct 22, 2020
@dengziming

dengziming commented Nov 6, 2020

Copy link
Copy Markdown
Member Author

also ping @chia7712 to have a look

@chia7712

chia7712 commented Dec 6, 2020

Copy link
Copy Markdown
Member

Seems like we could just get rid of this. The call to trace will already check isTraceEnabled before building the message, so I'm not sure we're saving anything.

The check was removed by ed8b031#diff-d886593f0d0ebeee617016c3dfa5b5d2981d7a8f776bac939416615ba50a001eR51. We do build the message when calling msgWithLogIdent. This is not related to this PR but we can add the check back to Logging#trace if we want to avoid checking isTraceEnabled from caller.

@chia7712

chia7712 commented Dec 9, 2020

Copy link
Copy Markdown
Member

@dengziming The docs fix LGTM. Maybe we can merge the code about docs fix. The issue about isTraceEnabled can be addressed in another PR. WDYT?

@dengziming

dengziming commented Dec 9, 2020

Copy link
Copy Markdown
Member Author

@dengziming The docs fix LGTM. Maybe we can merge the code about docs fix. The issue about isTraceEnabled can be addressed in another PR. WDYT?
Thank you for your suggestions, I changed the code and also changed the title.

@dengziming dengziming changed the title MINOR: Fix some java docs and move repeated if condition to the method MINOR: Fix some java docs of ReplicaStateMachine Dec 9, 2020
chia7712
chia7712 approved these changes Dec 9, 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.

@dengziming Thanks for your patch. LGTM

@chia7712 chia7712 merged commit 8e82eaa into apache:trunk Dec 10, 2020
@chia7712

chia7712 commented Dec 10, 2020

Copy link
Copy Markdown
Member

@dengziming Thanks for your patch. Feel free to open PR to handle isTraceEnabled

@dengziming dengziming deleted the MINOR-java-docs branch October 8, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hachikuji hachikuji hachikuji left review comments

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

3 participants

@dengziming @chia7712 @hachikuji