You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "smjn (via GitHub)" <gi...@apache.org> on 2024/08/14 15:12:55 UTC
[PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
smjn opened a new pull request, #16883:
URL: https://github.com/apache/kafka/pull/16883
There is a lot of code in `group-coordinator` which is not share/consumer/classic group specific.
Since we are introducing a `share-coordinator` as part of KIP-932 (in a new module), it would make sense to get the common coordinator functionality into a separate common coordinator module so that `share-coordinator` need not depend on `group-coordinator`.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718430521
##########
settings.gradle:
##########
@@ -57,6 +57,7 @@ include 'clients',
'connect:runtime',
'connect:test-plugins',
'connect:transforms',
+ 'coordinator-common',
Review Comment:
AFAIK, all the subprojects/modules for the kafka project must be declared in settings.gradle
Don't think this has anything to do with `clients` module.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718513125
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/metrics/CoordinatorRuntimeMetrics.java:
##########
@@ -14,9 +14,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group.metrics;
+package org.apache.kafka.coordinator.common.metrics;
-import org.apache.kafka.coordinator.group.runtime.CoordinatorRuntime.CoordinatorState;
+import org.apache.kafka.coordinator.common.runtime.CoordinatorRuntime.CoordinatorState;
import java.util.function.Supplier;
Review Comment:
The comment on line 24 would be better if it just said "the coordinator runtimes".
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718974680
##########
checkstyle/import-control-coordinator-common.xml:
##########
@@ -0,0 +1,78 @@
+<!DOCTYPE import-control PUBLIC
+ "-//Puppy Crawl//DTD Import Control 1.1//EN"
+ "http://www.puppycrawl.com/dtds/import_control_1_1.dtd">
+<!--
+ 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.
+-->
+
+<import-control pkg="org.apache.kafka">
+
+ <!-- THINK HARD ABOUT THE LAYERING OF THE PROJECT BEFORE CHANGING THIS FILE -->
+
+ <!-- common library dependencies -->
+ <allow pkg="java" />
+ <allow pkg="org.slf4j" />
+ <allow pkg="org.junit" />
+ <allow pkg="org.mockito" />
+ <!-- no one depends on the server -->
Review Comment:
Should it be `no one depends on the coordinator-common`?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718427379
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/MultiThreadedEventProcessorTest.java:
##########
@@ -45,7 +46,6 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.mock;
Review Comment:
No, remnant of automated refactoring. Will fix
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719774249
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRecord.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group;
+package org.apache.kafka.coordinator.common.runtime;
Review Comment:
I will use it
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1720978277
##########
settings.gradle:
##########
@@ -57,6 +57,7 @@ include 'clients',
'connect:runtime',
'connect:test-plugins',
'connect:transforms',
+ 'coordinator-common',
Review Comment:
@smjn thanks for sharing. sounds good
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719664053
##########
checkstyle/import-control-coordinator-common.xml:
##########
@@ -0,0 +1,78 @@
+<!DOCTYPE import-control PUBLIC
+ "-//Puppy Crawl//DTD Import Control 1.1//EN"
+ "http://www.puppycrawl.com/dtds/import_control_1_1.dtd">
+<!--
+ 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.
+-->
+
+<import-control pkg="org.apache.kafka">
+
+ <!-- THINK HARD ABOUT THE LAYERING OF THE PROJECT BEFORE CHANGING THIS FILE -->
+
+ <!-- common library dependencies -->
+ <allow pkg="java" />
+ <allow pkg="org.slf4j" />
+ <allow pkg="org.junit" />
+ <allow pkg="org.mockito" />
+ <!-- no one depends on the server -->
Review Comment:
I think you are right 👍
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719731148
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/CoordinatorRecord.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group;
+package org.apache.kafka.coordinator.common;
Review Comment:
I wonder if we should move `CoordinatorRecord`, `CoordinatorMetrics`, `CoordinatorMetricsShard` and `CoordinatorRuntimeMetrics` to the `runtime` package too. They are all generic classes which are part of the runtime framework in my opinion. What do you think?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719734149
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/MockCoordinatorTimer.java:
##########
@@ -43,7 +43,7 @@ public static class ScheduledTimeout<T, U> {
public final long deadlineMs;
public final TimeoutOperation<T, U> operation;
- ScheduledTimeout(
+ public ScheduledTimeout(
Review Comment:
Are those changes necessary?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "jeffkbkim (via GitHub)" <gi...@apache.org>.
jeffkbkim commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718390365
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/SnapshottableCoordinatorTest.java:
##########
@@ -14,17 +14,16 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group.runtime;
+package org.apache.kafka.coordinator.common.runtime;
import org.apache.kafka.common.TopicPartition;
import org.apache.kafka.common.utils.LogContext;
import org.apache.kafka.common.utils.MockTime;
-import org.apache.kafka.coordinator.group.MockCoordinatorTimer;
+import org.apache.kafka.coordinator.common.MockCoordinatorTimer;
import org.apache.kafka.timeline.SnapshotRegistry;
import org.junit.jupiter.api.Test;
-import static org.apache.kafka.coordinator.group.runtime.CoordinatorRuntimeTest.MockCoordinatorShard;
Review Comment:
ditto here
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/MultiThreadedEventProcessorTest.java:
##########
@@ -45,7 +46,6 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.mock;
Review Comment:
any reason we're changing this?
##########
settings.gradle:
##########
@@ -57,6 +57,7 @@ include 'clients',
'connect:runtime',
'connect:test-plugins',
'connect:transforms',
+ 'coordinator-common',
Review Comment:
can you help me understand what this does? my understanding is we should only disclose group-coordinator-api to the clients module
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##########
@@ -3117,11 +3117,11 @@ public void testSessionTimeoutExpiration() {
context.assertSessionTimeout(groupId, memberId, 45000);
// Advance time past the session timeout.
- List<ExpiredTimeout<Void, CoordinatorRecord>> timeouts = context.sleep(45000 + 1);
+ List<MockCoordinatorTimer.ExpiredTimeout<Void, CoordinatorRecord>> timeouts = context.sleep(45000 + 1);
Review Comment:
can we import
```
import org.apache.kafka.coordinator.group.MockCoordinatorTimer.ExpiredTimeout;
import org.apache.kafka.coordinator.group.MockCoordinatorTimer.ScheduledTimeout;
```
instead? some of the lines become too long
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntimeTest.java:
##########
@@ -629,8 +629,8 @@ public void testScheduleLoading() {
.withEventProcessor(new DirectEventProcessor())
.withPartitionWriter(writer)
.withCoordinatorShardBuilderSupplier(supplier)
- .withCoordinatorRuntimeMetrics(mock(GroupCoordinatorRuntimeMetrics.class))
- .withCoordinatorMetrics(mock(GroupCoordinatorMetrics.class))
+ .withCoordinatorRuntimeMetrics(Mockito.mock(CoordinatorRuntimeMetrics.class))
+ .withCoordinatorMetrics(Mockito.mock(CoordinatorMetrics.class))
Review Comment:
for all of the mock related changes, can't we use `import static org.mockito.Mockito.when;` in L103?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on PR #16883:
URL: https://github.com/apache/kafka/pull/16883#issuecomment-2291303027
> thanks! left some comments.
>
> also, i was wondering if CoordinatorRecordSerde and CoordinatorRecordSerde should now be prefixed with `Group`
We can - but they are already in `group` package, hence might be redundant.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719402485
##########
checkstyle/import-control-coordinator-common.xml:
##########
@@ -0,0 +1,78 @@
+<!DOCTYPE import-control PUBLIC
+ "-//Puppy Crawl//DTD Import Control 1.1//EN"
+ "http://www.puppycrawl.com/dtds/import_control_1_1.dtd">
+<!--
+ 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.
+-->
+
+<import-control pkg="org.apache.kafka">
+
+ <!-- THINK HARD ABOUT THE LAYERING OF THE PROJECT BEFORE CHANGING THIS FILE -->
+
+ <!-- common library dependencies -->
+ <allow pkg="java" />
+ <allow pkg="org.slf4j" />
+ <allow pkg="org.junit" />
+ <allow pkg="org.mockito" />
+ <!-- no one depends on the server -->
Review Comment:
@apoorvmittal10
No, that might be incorrect - other modules will depend on `coordinator-common`. Actually this comment exists in almost all checkstyle files.
Basically, we are disallowing accessing kafka package such that `import org.apache.kafka.ABCD` is not allowed and this is not module specific.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on PR #16883:
URL: https://github.com/apache/kafka/pull/16883#issuecomment-2292901492
> You also might want to consider correcting suppressions.xml to have classes under coordinator-runtime classification. Though it doesn't make any logical difference but just categorization.
Could you please clarify? The PR does not introduce any change in suppressions.xml.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719738265
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java:
##########
@@ -1775,7 +1775,7 @@ public void onHighWatermarkUpdated(
/**
* 16KB. Used for initial buffer size for write operations.
*/
- static final int MIN_BUFFER_SIZE = 16384;
+ public static final int MIN_BUFFER_SIZE = 16384;
Review Comment:
No, it is only required within coordinator-common as of now
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719772554
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRecord.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group;
+package org.apache.kafka.coordinator.common.runtime;
Review Comment:
Are you going to reuse CoordinatorRecord for the share coordinator? If not, it may be better to keep it in the group-coordinator module for now as it may be specific to the implementation of the coordinator.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719737864
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/MockCoordinatorTimer.java:
##########
@@ -43,7 +43,7 @@ public static class ScheduledTimeout<T, U> {
public final long deadlineMs;
public final TimeoutOperation<T, U> operation;
- ScheduledTimeout(
+ public ScheduledTimeout(
Review Comment:
Yes, it is used in group-coordinator
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719774249
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRecord.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.kafka.coordinator.group;
+package org.apache.kafka.coordinator.common.runtime;
Review Comment:
I will use it. The class is very general. I have moved the test class for the same as well.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1718975114
##########
checkstyle/import-control-coordinator-common.xml:
##########
@@ -0,0 +1,78 @@
+<!DOCTYPE import-control PUBLIC
+ "-//Puppy Crawl//DTD Import Control 1.1//EN"
+ "http://www.puppycrawl.com/dtds/import_control_1_1.dtd">
+<!--
+ 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.
+-->
+
+<import-control pkg="org.apache.kafka">
+
+ <!-- THINK HARD ABOUT THE LAYERING OF THE PROJECT BEFORE CHANGING THIS FILE -->
+
+ <!-- common library dependencies -->
+ <allow pkg="java" />
+ <allow pkg="org.slf4j" />
+ <allow pkg="org.junit" />
+ <allow pkg="org.mockito" />
+ <!-- no one depends on the server -->
+ <disallow pkg="kafka" />
+
+ <!-- anyone can use public classes -->
+ <!-- anyone can use public classes -->
Review Comment:
Are they 2 repeated comments?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1719733751
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java:
##########
@@ -1775,7 +1775,7 @@ public void onHighWatermarkUpdated(
/**
* 16KB. Used for initial buffer size for write operations.
*/
- static final int MIN_BUFFER_SIZE = 16384;
+ public static final int MIN_BUFFER_SIZE = 16384;
Review Comment:
Is this change necessary?
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "smjn (via GitHub)" <gi...@apache.org>.
smjn commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1720959346
##########
settings.gradle:
##########
@@ -57,6 +57,7 @@ include 'clients',
'connect:runtime',
'connect:test-plugins',
'connect:transforms',
+ 'coordinator-common',
Review Comment:
Mainly 2 reasons for the current name.
1. The new parent coordinator module does not do anything useful on its own. `common-coordinator` has the connotation that this module serves a well defined purpose like group coordinator but that is not the case. It just runs the framework classes and the useful work is only done when a cooperating coordinator provides concrete implementations. You are also right in pointing out the server-common nomenclature. But in the same spirit common-server has a very different connotation and seems incorrect.
2. The original author of the group-coordinator suggested the name.
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16883:
URL: https://github.com/apache/kafka/pull/16883
--
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
Re: [PR] KAFKA-17342: Moved common coordinator code to separate module. [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16883:
URL: https://github.com/apache/kafka/pull/16883#discussion_r1720955414
##########
settings.gradle:
##########
@@ -57,6 +57,7 @@ include 'clients',
'connect:runtime',
'connect:test-plugins',
'connect:transforms',
+ 'coordinator-common',
Review Comment:
I guess the next coordinator-related module is called `share-coordinator`, and there is existent module called `group-coordinator`. Hence, have we consider the name: `common-coordinator`? Or you want to align the naming with `server-common`
--
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