KAFKA-18453: Add StreamsTopology class to group coordinator #18446
Conversation
Adds a class that represent the topology of a Streams group to the group coordinator.
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/StreamsTopology.java
Outdated
Show resolved
Hide resolved
| import java.util.stream.Stream; | ||
|
|
||
| /** | ||
| * Contains all information related to a topology of a Streams group. |
There was a problem hiding this comment.
We should specify that this is the requested topology that has not been configured according to the topics available on the broker. So differentiate to ConfiguredTopology. It doesn't really contain "all information related to a topology".
| /** | ||
| * Contains all information related to a topology of a Streams group. | ||
| * <p> | ||
| * This class is immutable and is fully backed by records stored in the __consumer_offsets topic. |
There was a problem hiding this comment.
I know I wrote that, so I'm talking to myself here, but the subtopologies objects aren't actually immutable. We don't want to mutate it, but saying it's immutable is probably misleady.
There was a problem hiding this comment.
Fair enough! I ensured that the map is unmodifiable at least.
There was a problem hiding this comment.
Fair enough! I ensured that the map is unmodifiable at least.
| * Contains all information related to a topology of a Streams group. | ||
| * <p> | ||
| * This class is immutable and is fully backed by records stored in the __consumer_offsets topic. | ||
| * |
There was a problem hiding this comment.
I think it's also useful to specify, that the main purpose of this class compared to StreamGroupTopologyValue is that we have a map for looking up a subtopology by name in constant time.
| Map<String, Subtopology> subtopologies) { | ||
|
|
||
| /** | ||
| * Returns the set of topics required by the topology. |
There was a problem hiding this comment.
These javadocs seem somewhat void of content. They say nothing that isn't implied by the function name. If we are going to put these javadocs, please specify what it means to be "required".
| /** | ||
| * Creates a instance of StreamsTopology from a StreamsGroupTopologyValue record. | ||
| * | ||
| * @param record StreamsGroupTopologyValue record |
There was a problem hiding this comment.
I think most other classes use full sentences, with initial character capitalized and trailing period in @param and @return. Applies to several javadoc comments here.
| )); | ||
| } | ||
|
|
||
| private Subtopology mkSubtopology2() { |
There was a problem hiding this comment.
I personally preferred the unit tests when they were self-contained, which is easier to understand. There is no benefit to making this a separate method, and mkSubtopology2 doesn't mean anything.
There was a problem hiding this comment.
I disagree.
There is at least the benefit to not duplicate the code for the creation of the subtopology.
What do you mean with
mkSubtopology2doesn't mean anything
You look once at the mkSubtopologyX methods and understand the created subtopologies and then when you look at the tests you do not need to understand the subtopology again for each test.
There was a problem hiding this comment.
Okay, as stated this is just my preference - self-contained tests go a long way, and sharing data between tests easily becomes hard to deal with. This class won't grow much, so either way is probably fine. Agreed to keep it like this.
Adds a class that represent the topology of a Streams group to the group coordinator.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)