jeffkbkim commented on code in PR #17694: URL: https://github.com/apache/kafka/pull/17694#discussion_r1829908628
########## group-coordinator/src/main/resources/common/message/ConsumerGroupRegularExpressionKey.json: ########## @@ -0,0 +1,28 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// KIP-848 is in development. This schema is subject to non-backwards-compatible changes. +{ + "type": "data", + "name": "ConsumerGroupRegularExpressionKey", Review Comment: should we add these record types to the kip? ########## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordHelpers.java: ########## @@ -408,6 +411,61 @@ public static CoordinatorRecord newConsumerGroupCurrentAssignmentTombstoneRecord ); } + /** + * Creates a ConsumerGroupRegularExpression record. + * + * @param groupId The consumer group id. + * @param regex The regular expression. + * @param metadata The metadata associated with the regular expression. + * @return The record. + */ + public static CoordinatorRecord newConsumerGroupRegularExpressionRecord( + String groupId, + String regex, + ResolvedRegularExpression metadata Review Comment: nit: referring ResolvedRegularExpression as metadata throughout the code was a bit confusing at first glance. I can't think of a better name though 😅 maybe ResolvedRegexMetadata? ########## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java: ########## @@ -872,6 +875,13 @@ public void replay( ); break; + case 15: Review Comment: should we start a mapping for these key record versions at some point? also, we only care about this record version from the __consumer_offsets topic's perspective right? ########## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/consumer/ResolvedRegularExpression.java: ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.coordinator.group.modern.consumer; + +import java.util.Collections; +import java.util.Objects; +import java.util.Set; + +/** + * The metadata associated with a regular expression in a Consumer Group. + */ +public class ResolvedRegularExpression { Review Comment: to understand the usage of this class and the new record: ConsumerGroupRegularExpressionValue is the log representation of ResolvedRegularExpression. It mainly stores the list of topics that have been resolved with the regex pattern with the latest topic metadata We store a mapping of regex pattern -> ResolvedRegularExpression (set of topics) which will be used as a cache Subsequent PRs will * try-match existing regex patterns to topics when topic metadata changes * create & store new record in log if resolved topics have changed let me know this is correct -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
