[GitHub] [kafka] dajac commented on a diff in pull request #13511: KAFKA-14869: ignore unknown record types for coordinators

["dajac (via GitHub)" <gi...@apache.org>]

dajac commented on code in PR #13511:
URL: https://github.com/apache/kafka/pull/13511#discussion_r1163731655


##########
clients/src/main/java/org/apache/kafka/common/protocol/MessageUtil.java:
##########
@@ -231,4 +231,38 @@ public static byte[] toVersionPrefixedBytes(final short version, final Message m
                 buffer.limit() == buffer.array().length) return buffer.array();
         else return Utils.toArray(buffer);
     }
+
+    // Should only be used for testing
+    public static byte[] messageWithUnknownVersion() {

Review Comment:
   It is a bit awkward to have this in the `src` directory. I am not really sure to understand why we need it as well. I suppose that we could use any other `Message` like record from `test`, isn't it?



##########
core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala:
##########
@@ -1145,17 +1150,22 @@ object GroupMetadataManager {
    * @param buffer input byte-buffer
    * @return an OffsetKey or GroupMetadataKey object from the message
    */
-  def readMessageKey(buffer: ByteBuffer): BaseKey = {
+  def readMessageKey(buffer: ByteBuffer): Option[BaseKey] = {

Review Comment:
   Have you considered introducing an `UnknownKey` which extends `BaseKey`? The `UnknownKey` could carry on the version and even the bytes. It seems that the change would be less disruptive like this.



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

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org