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

Replaced unnecessary isDefined and get on option values with fold #2050

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
3 participants
@himani1
Copy link
Contributor

commented Oct 21, 2016

No description provided.

@hachikuji
Copy link
Contributor

left a comment

Thanks for the patch. Left a few suggestions.

@@ -141,10 +141,10 @@ case class PartitionMetadata(partitionId: Int,
override def toString: String = {
val partitionMetadataString = new StringBuilder
partitionMetadataString.append("\tpartition " + partitionId)
partitionMetadataString.append("\tleader: " + (if(leader.isDefined) leader.get.toString else "none"))
partitionMetadataString.append("\tleader: " + leader.fold("none")(leader => leader.toString))

This comment has been minimized.

Copy link
@hachikuji

hachikuji Oct 27, 2016

Contributor

Would leader.getOrElse("none") work as well?

partitionMetadataString.append("\treplicas: " + replicas.mkString(","))
partitionMetadataString.append("\tisr: " + isr.mkString(","))
partitionMetadataString.append("\tisUnderReplicated: %s".format(if(isr.size < replicas.size) "true" else "false"))
partitionMetadataString.append("\tisUnderReplicated: %s".format((isr.size < replicas.size).toString))

This comment has been minimized.

Copy link
@hachikuji

hachikuji Oct 27, 2016

Contributor

Would the toString() be necessary if we used + like the lines above? I thought the toString is implicit.

@@ -189,7 +189,7 @@ object DumpLogSegments {
val wrapperMessageOpt = shallowIter.find(_.offset >= entry.offset + timeIndex.baseOffset)
if (!wrapperMessageOpt.isDefined || wrapperMessageOpt.get.offset != entry.offset + timeIndex.baseOffset) {
timeIndexDumpErrors.recordShallowOffsetNotFound(file, entry.offset + timeIndex.baseOffset,
{if (wrapperMessageOpt.isDefined) wrapperMessageOpt.get.offset else -1})
{wrapperMessageOpt.fold(-1.toLong)(wrapperMessage => wrapperMessage.offset)})

This comment has been minimized.

Copy link
@hachikuji

hachikuji Oct 27, 2016

Contributor

Maybe we could drop the braces and use _.offset?

@@ -189,7 +189,7 @@ object DumpLogSegments {
val wrapperMessageOpt = shallowIter.find(_.offset >= entry.offset + timeIndex.baseOffset)
if (!wrapperMessageOpt.isDefined || wrapperMessageOpt.get.offset != entry.offset + timeIndex.baseOffset) {

This comment has been minimized.

Copy link
@hachikuji

hachikuji Oct 27, 2016

Contributor

Since we're in here already, maybe we could improve this using match?

This comment has been minimized.

Copy link
@himani1

himani1 Oct 28, 2016

Author Contributor

@hachikuji Why do you want to use match for boolean expressions?
Since match is efficient but for compactness and performance shouldn't we be using if/else?

This comment has been minimized.

Copy link
@hachikuji

hachikuji Oct 28, 2016

Contributor

Hmm... I'd be really surprised if the performance of match on an Option was any worse than the explicit if checks (wouldn't the compiler end up generating roughly equivalent code?). And I'm not sure performance is too critical for a tool like this anyway. Maybe we ought to focus on readability instead? It's not too important though, so feel free to ignore.

This comment has been minimized.

Copy link
@himani1

himani1 Oct 28, 2016

Author Contributor

@hachikuji : Since we are focussing more on readability I have updatedif/else to match Please have a look and let me know if any further change is required.

himani1 added some commits Oct 28, 2016

// since it is a sparse file, in the event of a crash there may be many zero entries, stop if we see one
if (entry.offset == 0 && i > 0)
return
wrapperMessageOpt match {

This comment has been minimized.

Copy link
@hachikuji

hachikuji Nov 1, 2016

Contributor

I this we could collapse the offset check into the same match, i.e.:

        case None =>
          timeIndexDumpErrors.recordShallowOffsetNotFound(file, entry.offset + timeIndex.baseOffset, -1.toLong)
        case Some(wrapperMessage) if wrapperMessage.offset != entry.offset + timeIndex.baseOffset =>
          timeIndexDumpErrors.recordShallowOffsetNotFound(file, entry.offset + timeIndex.baseOffset, wrapperMessage.offset)
        case Some(wrapperMessage) =>

Maybe that's a little more concise?

This comment has been minimized.

Copy link
@himani1

himani1 Nov 2, 2016

Author Contributor

@hachikuji Yes, Thanks for suggesting this. It would make the code more readable. I have done these changes. Please have a look.

@hachikuji

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

LGTM. Thanks for the patch! By the way, a quick note on conventions. Usually we prefix the title of minor patches like this with "MINOR: ". More serious bugs or improvements are usually accompanied by a JIRA, and we then use the ticket number as the prefix (e.g. "KAFKA-3299: Fix blah blah"). If you'd like to start working on larger patches, ping me or someone on the dev list with your apache id, and we'll add you as a contributor so that you can assign JIRAs to yourself.

@himani1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

@hachikuji : Thanks . I'll follow this conventions in my future PRs. My apache id is himani1

@asfgit asfgit closed this in 2959bc2 Nov 2, 2016

@himani1 himani1 deleted the himani1:refactored_code branch Nov 2, 2016

@hachikuji

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

@himani1 Thanks, I've added you as a contributor.

partitionMetadataString.append("\treplicas: " + replicas.mkString(","))
partitionMetadataString.append("\tisr: " + isr.mkString(","))
partitionMetadataString.append("\tisUnderReplicated: %s".format(if(isr.size < replicas.size) "true" else "false"))
partitionMetadataString.append("\tisUnderReplicated: %s" + (isr.size < replicas.size))

This comment has been minimized.

Copy link
@ijuma

ijuma Nov 2, 2016

Contributor

There's a bug introduced here. We removed the format call but left %s.

This comment has been minimized.

Copy link
@himani1

himani1 Nov 2, 2016

Author Contributor

@ijuma : Thanks for pointing it out. I have fixed this bug in my new PR. Please have a look.

This comment has been minimized.

Copy link
@hachikuji

hachikuji Nov 3, 2016

Contributor

Good catch, @ijuma!

@himani1 himani1 restored the himani1:refactored_code branch Nov 2, 2016

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.