Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-3768: Replace all pattern match on boolean value by if/else block. #1445

Closed

Conversation

Projects
None yet
2 participants
@satendrakumar
Copy link
Contributor

commented May 29, 2016

@satendrakumar

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

@ijuma Please review it. Let me know in any changes required.

debug("Some broker in ISR is alive for %s. Select %d from ISR %s to be the leader."
.format(topicAndPartition, newLeader, liveBrokersInIsr.mkString(",")))
new LeaderAndIsr(newLeader, currentLeaderEpoch + 1, liveBrokersInIsr.toList, currentLeaderIsrZkPathVersion + 1)
val newLeaderAndIsr = if (liveBrokersInIsr.isEmpty) {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

The layout is a bit nicer if the if is moved to the next line so that it's aligned with the else statement.

// leader is alive
partitionState.put(topicPartition, OnlinePartition)
} else {
partitionState.put(topicPartition, OfflinePartition)
}

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

No need for braces.

replicaState.put(partitionAndReplica, ReplicaDeletionIneligible)
if (controllerContext.liveBrokerIds.contains(replicaId)) {
replicaState.put(partitionAndReplica, OnlineReplica)
} else {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

No need for braces.

buffer.get(bytes, off, realLen)
realLen
case false => -1
if(buffer.hasRemaining) {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Space missing after if

val realLen = math.min(len, buffer.remaining())
buffer.get(bytes, off, realLen)
realLen
}else {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Space missing before else and no braces needed for the else.

(name, mbsc.getAttributes(name, mbean.getAttributes.map(_.getName)).size)}.toMap
if (attributesWhitelistExists) {
queries.map((_, attributesWhitelist.get.size)).toMap
} else { names.map{(name: ObjectName) =>

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

names... should be moved to the next line.

case false => names.map{(name: ObjectName) =>
val mbean = mbsc.getMBeanInfo(name)
(name, mbsc.getAttributes(name, mbean.getAttributes.map(_.getName)).size)}.toMap
if (attributesWhitelistExists) {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Braces not needed.

val secureOpt: String = if (secondZk.isSecure) {
firstZk.createPersistentPath(ZkUtils.ConsumersPath)
"secure"
}else {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Space missing before else.

case false =>
secondZk.createPersistentPath(ZkUtils.ConsumersPath)
"unsecure"
val secureOpt: String = if (secondZk.isSecure) {

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

The layout is a bit nicer if the if is moved to the following line. There's also two empty spaces before = instead of one (a previous issue).

list.size == 2
else
list.size == 1
}

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Braces are not needed.

case true => isAclSecure
case false => isAclUnsecure
})
if(secure)

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 1, 2016

Contributor

Space missing after if.

Update code indentation
KAFKA-3768
@satendrakumar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

@ijuma Thanks for review. I have updated code as your comments.

partitionState.put(topicPartition, OfflinePartition)
}
if (controllerContext.liveBrokerIds.contains(currentLeaderIsrAndEpoch.leaderAndIsr.leader))
// leader is alive

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 2, 2016

Contributor

Should be indented more.

This comment has been minimized.

Copy link
@satendrakumar

satendrakumar Jun 3, 2016

Author Contributor

@ijuma I did not understand your point("indented more"). Could you please give an example

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 3, 2016

Contributor

The comment is on the line after an if so it should be indented, e.g.:

if (controllerContext.liveBrokerIds.contains(currentLeaderIsrAndEpoch.leaderAndIsr.leader))
  // leader is alive
  partitionState.put(topicPartition, OnlinePartition)
else
  partitionState.put(topicPartition, OfflinePartition)
else
// mark replicas on dead brokers as failed for topic deletion, if they belong to a topic to be deleted.
// This is required during controller failover since during controller failover a broker can go down,
// so the replicas on that broker should be moved to ReplicaDeletionIneligible to be on the safer side.

This comment has been minimized.

Copy link
@ijuma

ijuma Jun 2, 2016

Contributor

Should be indented more.

@satendrakumar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2016

@ijuma Thanks for explanation. That's make more sense for code readability. I have updated code

@ijuma

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2016

LGTM, merging to trunk.

@asfgit asfgit closed this in ab35606 Jun 4, 2016

@satendrakumar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2016

@ijuma Thanks for your help.

@satendrakumar satendrakumar deleted the satendrakumar:remove_boolean_pattern_match branch Jun 5, 2016

kamalcph pushed a commit to kamalcph/kafka that referenced this pull request Jun 28, 2016

KAFKA-3768; Replace all pattern match on boolean value by if/else block.
Author: Satendra kumar <satendra@knoldus.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes apache#1445 from satendrakumar06/remove_boolean_pattern_match
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.