You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/04/25 13:24:11 UTC
[GitHub] [kafka] dengziming opened a new pull request #8552: [MINOR]: Fix some java docs
dengziming opened a new pull request #8552:
URL: https://github.com/apache/kafka/pull/8552
Modify the java docs of ZkReplicaStateMachine, the java docs is not in agreement with `OfflineReplica.validPreviousStates` and `OnlineReplica.validPreviousStates`
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #8552: MINOR: Fix some java docs and move repeated `if condition` to the method
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#issuecomment-722949912
also ping @chia7712 to have a look
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #8552: MINOR: Fix some java docs and move repeated `if condition` to the method
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#issuecomment-741639187
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #8552: [MINOR]: Fix some java docs and move repeated `if conditioni` to the method
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#discussion_r436405820
##########
File path: core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
##########
@@ -423,9 +414,10 @@ class ZkReplicaStateMachine(config: KafkaConfig,
(result.toMap, partitionsWithNoLeaderAndIsrInZk)
}
- private def logSuccessfulTransition(logger: StateChangeLogger, replicaId: Int, partition: TopicPartition,
+ private def logSuccessfulTransition(traceEnabled: Boolean, logger: StateChangeLogger, replicaId: Int, partition: TopicPartition,
currState: ReplicaState, targetState: ReplicaState): Unit = {
- logger.trace(s"Changed state of replica $replicaId for partition $partition from $currState to $targetState")
+ if (traceEnabled)
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #8552: MINOR: Fix some java docs of ReplicaStateMachine
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #8552:
URL: https://github.com/apache/kafka/pull/8552
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #8552: MINOR: Fix some java docs of ReplicaStateMachine
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#issuecomment-742208769
@dengziming Thanks for your patch. Feel free to open PR to handle ```isTraceEnabled```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #8552: MINOR: Fix some java docs and move repeated `if condition` to the method
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#issuecomment-739501704
> 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 https://github.com/apache/kafka/commit/ed8b0315a6c3705b2a163ce3ab4723234779264f#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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #8552: MINOR: Fix some java docs and move repeated `if condition` to the method
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#issuecomment-741646665
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #8552: [MINOR]: Fix some java docs and move repeated `if conditioni` to the method
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #8552:
URL: https://github.com/apache/kafka/pull/8552#discussion_r436456482
##########
File path: core/src/main/scala/kafka/controller/ReplicaStateMachine.scala
##########
@@ -423,9 +414,10 @@ class ZkReplicaStateMachine(config: KafkaConfig,
(result.toMap, partitionsWithNoLeaderAndIsrInZk)
}
- private def logSuccessfulTransition(logger: StateChangeLogger, replicaId: Int, partition: TopicPartition,
+ private def logSuccessfulTransition(traceEnabled: Boolean, logger: StateChangeLogger, replicaId: Int, partition: TopicPartition,
currState: ReplicaState, targetState: ReplicaState): Unit = {
- logger.trace(s"Changed state of replica $replicaId for partition $partition from $currState to $targetState")
+ if (traceEnabled)
Review comment:
Done, also ping @stanislavkozlovski
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org