KAFKA-4103: Fix regression in DumpLogSegments offsets decoder #1807
Conversation
| @@ -257,7 +257,7 @@ object DumpLogSegments { | |||
| private def parseGroupMetadata(groupMetadataKey: GroupMetadataKey, payload: ByteBuffer) = { | |||
| val groupId = groupMetadataKey.key | |||
| val group = GroupMetadataManager.readGroupMessageValue(groupId, payload) | |||
| val protocolType = group.protocolType | |||
| val protocolType = group.protocolType.getOrElse("NONE") | |||
There was a problem hiding this comment.
This is really for empty groups. We could alternatively use "" as the absent protocol type.
There was a problem hiding this comment.
Wouldn't it be better to pattern match on the protocol type?
There was a problem hiding this comment.
We could I suppose. It will only be None when the group is empty, so maybe I can skip iterating over the members in that case.
There was a problem hiding this comment.
@ijuma It seemed a little more straightforward to use a default value, but let me know if you disagree.
There was a problem hiding this comment.
Not a big deal for this code and it was already merged, so fine to stay as is.
My reasoning is that you lose a bit of type safety by converting from Option[String] to a String sentinel value and then doing an equality comparison on it. We should never have a protocol with name "", so unlikely to matter here, but I generally prefer to keep the types for as long as possible as it helps avoid bugs as the code evolves.
As an aside, this regression would never have happened if Scala had a type safe equals by default (a number of libraries have a === which is a type safe version of == as a workaround).
There was a problem hiding this comment.
@ijuma Makes sense. I agree it would be awesome if there was stricter type checking for == by default. I can't think of any case where I wouldn't want that.
|
LGTM |
No description provided.