You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mimaison (via GitHub)" <gi...@apache.org> on 2024/11/15 17:40:57 UTC
[PR] MINOR: Various cleanups in coordinator modules [kafka]
mimaison opened a new pull request, #17828:
URL: https://github.com/apache/kafka/pull/17828
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17828:
URL: https://github.com/apache/kafka/pull/17828#discussion_r1846204223
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -83,15 +83,15 @@ public void testBasicOperations() {
assertNull(accumulator.poll());
List<MockEvent> events = Arrays.asList(
Review Comment:
I kept `Arrays.asList()` for consistency with the other test below that requires a mutable list.
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #17828:
URL: https://github.com/apache/kafka/pull/17828#discussion_r1844258516
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java:
##########
@@ -120,10 +120,10 @@ public class GroupCoordinatorConfig {
public static final String CONSUMER_GROUP_ASSIGNORS_CONFIG = "group.consumer.assignors";
public static final String CONSUMER_GROUP_ASSIGNORS_DOC = "The server side assignors as a list of full class names. The first one in the list is considered as the default assignor to be used in the case where the consumer does not specify an assignor.";
- public static final List<String> CONSUMER_GROUP_ASSIGNORS_DEFAULT = Collections.unmodifiableList(Arrays.asList(
+ public static final List<String> CONSUMER_GROUP_ASSIGNORS_DEFAULT = List.of(
UniformAssignor.class.getName(),
RangeAssignor.class.getName()
Review Comment:
The indentation is actually wrong here too.
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17828:
URL: https://github.com/apache/kafka/pull/17828#discussion_r1846205074
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -83,15 +83,15 @@ public void testBasicOperations() {
assertNull(accumulator.poll());
List<MockEvent> events = Arrays.asList(
- new MockEvent(1, 0),
- new MockEvent(1, 1),
- new MockEvent(1, 2),
- new MockEvent(2, 0),
- new MockEvent(2, 1),
- new MockEvent(2, 3),
- new MockEvent(3, 0),
- new MockEvent(3, 1),
- new MockEvent(3, 2)
+ new MockEvent(1, 0),
Review Comment:
My bad, IntelliJ must have reformatted that, undoing
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -83,15 +83,15 @@ public void testBasicOperations() {
assertNull(accumulator.poll());
List<MockEvent> events = Arrays.asList(
- new MockEvent(1, 0),
- new MockEvent(1, 1),
- new MockEvent(1, 2),
- new MockEvent(2, 0),
- new MockEvent(2, 1),
- new MockEvent(2, 3),
- new MockEvent(3, 0),
- new MockEvent(3, 1),
- new MockEvent(3, 2)
+ new MockEvent(1, 0),
Review Comment:
My bad, IntelliJ must have reformatted that, reverting these changes
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #17828:
URL: https://github.com/apache/kafka/pull/17828#discussion_r1844251020
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -83,15 +83,15 @@ public void testBasicOperations() {
assertNull(accumulator.poll());
List<MockEvent> events = Arrays.asList(
- new MockEvent(1, 0),
- new MockEvent(1, 1),
- new MockEvent(1, 2),
- new MockEvent(2, 0),
- new MockEvent(2, 1),
- new MockEvent(2, 3),
- new MockEvent(3, 0),
- new MockEvent(3, 1),
- new MockEvent(3, 2)
+ new MockEvent(1, 0),
Review Comment:
Why did you change this? We use 4 spaces everywhere in this module.
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -118,9 +118,9 @@ public void testAddFirst() {
EventAccumulator<Integer, MockEvent> accumulator = new EventAccumulator<>();
List<MockEvent> events = Arrays.asList(
- new MockEvent(1, 0),
- new MockEvent(1, 1),
- new MockEvent(1, 2)
+ new MockEvent(1, 0),
Review Comment:
ditto.
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -83,15 +83,15 @@ public void testBasicOperations() {
assertNull(accumulator.poll());
List<MockEvent> events = Arrays.asList(
Review Comment:
We could use List.of here.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java:
##########
@@ -380,10 +380,9 @@ public Map<String, Integer> extractGroupConfigMap(ShareGroupConfig shareGroupCon
* Copy the subset of properties that are relevant to consumer group.
*/
public Map<String, Integer> extractConsumerGroupConfigMap() {
- Map<String, Integer> groupProps = new HashMap<>();
- groupProps.put(GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, consumerGroupSessionTimeoutMs());
- groupProps.put(GroupConfig.CONSUMER_HEARTBEAT_INTERVAL_MS_CONFIG, consumerGroupHeartbeatIntervalMs());
- return Collections.unmodifiableMap(groupProps);
+ return Map.of(
+ GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, consumerGroupSessionTimeoutMs(),
+ GroupConfig.CONSUMER_HEARTBEAT_INTERVAL_MS_CONFIG, consumerGroupHeartbeatIntervalMs());
Review Comment:
nit: To follow the style in the module.
```
return Map.of(
GroupConfig.CONSUMER_SESSION_TIMEOUT_MS_CONFIG, consumerGroupSessionTimeoutMs(),
GroupConfig.CONSUMER_HEARTBEAT_INTERVAL_MS_CONFIG, consumerGroupHeartbeatIntervalMs()
);
```
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -118,9 +118,9 @@ public void testAddFirst() {
EventAccumulator<Integer, MockEvent> accumulator = new EventAccumulator<>();
List<MockEvent> events = Arrays.asList(
Review Comment:
List.of.
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #17828:
URL: https://github.com/apache/kafka/pull/17828
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #17828:
URL: https://github.com/apache/kafka/pull/17828#discussion_r1846222122
##########
coordinator-common/src/test/java/org/apache/kafka/coordinator/common/runtime/EventAccumulatorTest.java:
##########
@@ -118,9 +118,9 @@ public void testAddFirst() {
EventAccumulator<Integer, MockEvent> accumulator = new EventAccumulator<>();
List<MockEvent> events = Arrays.asList(
Review Comment:
We do `Collections.reverse()` below which requires a mutable list.
--
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] MINOR: Various cleanups in coordinator modules [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17828:
URL: https://github.com/apache/kafka/pull/17828#issuecomment-2482531730
@m1a2st We've not started adopting `record` classes anywhere in the project yet. I think we should start using that feature but we can do it in another PR.
--
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