mjsax commented on code in PR #22213:
URL: https://github.com/apache/kafka/pull/22213#discussion_r3203903327
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfig.java:
##########
@@ -292,6 +298,11 @@ public final class GroupConfig extends AbstractConfig {
atLeast(0),
MEDIUM,
GroupCoordinatorConfig.STREAMS_GROUP_NUM_WARMUP_REPLICAS_DOC)
+ .define(STREAMS_RACK_AWARE_ASSIGNMENT_TAGS_CONFIG,
+ LIST,
+
GroupCoordinatorConfig.STREAMS_GROUP_RACK_AWARE_ASSIGNMENT_TAGS_DEFAULT,
+ MEDIUM,
+
GroupCoordinatorConfig.STREAMS_GROUP_RACK_AWARE_ASSIGNMENT_TAGS_DOC)
Review Comment:
I believe that `LIST` type does take care of this already? We have an
existing test for the "classic" client side config,
`StreamsConfigTest#shouldThrowExceptionWhenClientTagRackAwarenessIsConfiguredWithEmptyTag`,
and it should work the same broker side?
Might be good to add a test for it (maybe both, coordinator-config, and
group-config)?
What also reminds me, that we might want to also have guard on both configs
(especially the coordinator one -- for the group one, we can simply reject the
config change with an error?) to also ensure that the group-coordinator starts
up, even if `group.streams.rack.aware.assignment.tags` is set so something
invalid (and maybe fall back to default empty string). -- Would be good to hear
from one of @lucasbru @lianetm @AndrewJSchofield @dajac @squah-confluent about
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
