KAFKA-12648: MINOR - Add TopologyMetadata.Subtopology class for subtopology metadata #10676
Conversation
ableegoldman
changed the title
ableegoldman
force-pushed
the
MINOR-rename-Subtopology-to-SubtopologyDescription
branch
from
105eca2 to
502c6c7
Compare
ableegoldman
force-pushed
the
MINOR-rename-Subtopology-to-SubtopologyDescription
branch
from
4c46ddb to
9243a12
Compare
ableegoldman
force-pushed
the
MINOR-rename-Subtopology-to-SubtopologyDescription
branch
2 times, most recently
from
62bd924 to
b54f0d0
Compare
|
Ready for review @wcarlson5 @guozhangwang @rodesai |
ableegoldman
force-pushed
the
MINOR-rename-Subtopology-to-SubtopologyDescription
branch
from
b54f0d0 to
6245684
Compare
| new InternalTopologyBuilder.Subtopology(0, | ||
| Collections.singleton(expectedSourceNode))); | ||
| new SubtopologyDescription(0, | ||
| Collections.singleton(expectedSourceNode))); |
There was a problem hiding this comment.
nit: are these indents right?
There was a problem hiding this comment.
Yeah, looks like my IDE just reformatted this when it did the renaming.
| @@ -59,10 +58,6 @@ public TaskId(final int topicGroupId, final int partition, final String namedTop | |||
| } | |||
| } | |||
|
|
|||
| public Optional<String> namedTopology() { | |||
There was a problem hiding this comment.
If we don't have this could we add something like it to TaskMetadata?
There was a problem hiding this comment.
Hm, TaskMetadata is also a public class, right? I think we'll need to do something similar in that case, ie add an internal NamedTaskMetadata class that exposes the namedTopology and then you can access it via something like NamedTaskMetadata.extractNamedTopology(TaskMetadata)
It's unfortunate that TaskMetadata has the taskId field as a String, rather than as an actual TaskId object -- not sure what the motivation there was 😕 I might try to do a very small KIP to fix that, and then we won't have to bother with this at all (you can just use the internal NamedTaskId class/method to access the namedTopology
There was a problem hiding this comment.
It would be defiantly better to just fix the taskId in taskMetadata :)
There was a problem hiding this comment.
Alright, here we are: KIP-740: Use TaskId instead of String for the taskId field in TaskMetadata
That was probably the fastest/easiest KIP I've ever written
ableegoldman
changed the title
|
I'm going to merge this without another committer +1 since it's a minor PR with mostly renaming/wrapping classes. And the PR build is actually completely green for once 🥳 |
…e-allocations-lz4 * apache-github/trunk: (155 commits) KAFKA-12728: Upgrade gradle to 7.0.2 and shadow to 7.0.0 (apache#10606) KAFKA-12778: Fix QuorumController request timeouts and electLeaders (apache#10688) KAFKA-12754: Improve endOffsets for TaskMetadata (apache#10634) Rework on KAFKA-3968: fsync the parent directory of a segment file when the file is created (apache#10680) MINOR: set replication.factor to 1 to make StreamsBrokerCompatibilityService work with old broker (apache#10673) MINOR: prevent cleanup() from being called while Streams is still shutting down (apache#10666) KAFKA-8326: Introduce List Serde (apache#6592) KAFKA-12697: Add Global Topic and Partition count metrics to the Quorum Controller (apache#10679) KAFKA-12648: MINOR - Add TopologyMetadata.Subtopology class for subtopology metadata (apache#10676) MINOR: Update jacoco to 0.8.7 for JDK 16 support (apache#10654) MINOR: exclude all `src/generated` and `src/generated-test` (apache#10671) KAFKA-12772: Move all transaction state transition rules into their states (apache#10667) KAFKA-12758 Added `server-common` module to have server side common classes. (apache#10638) MINOR Removed copying storage libraries specifically as they are already copied. (apache#10647) KAFKA-5876: KIP-216 Part 4, Apply InvalidStateStorePartitionException for Interactive Queries (apache#10657) KAFKA-12747: Fix flakiness in shouldReturnUUIDsWithStringPrefix (apache#10643) MINOR: remove unnecessary placeholder from WorkerSourceTask#recordSent (apache#10659) MINOR: Remove unused `scalatest` definition from `dependencies.gradle` (apache#10655) MINOR: checkstyle version upgrade: 8.20 -> 8.36.2 (apache#10656) KAFKA-12464: minor code cleanup and additional logging in constrained sticky assignment (apache#10645) ...
There was a problem hiding this comment.
@ableegoldman do we have a KIP for this change since this is public APIs (not a fan to be picky really)... if not, at least we should document in the upgrade guides.
|
@guozhangwang Ah, shoot, I forgot that the interface name would be public of course..I'll just go ahead and revert that change. I believe the internal class name change should be fine but let me know if there's another name you would prefer |
|
Here: #10713 |
In #10676 we renamed the internal Subtopology class that implemented the TopologyDescription.Subtopology interface. By mistake, we also renamed the interface itself, which is a public API. This wasn't really the intended point of that PR, so rather than do a retroactive KIP, let's just reverse the renaming. Reviewers: Walker Carlson <wcarlson@confluent.io>, Guozhang Wang <guozhang@confluent.io>
Renames the existing
Subtopologyclass + interface toSubtopologyDescription, since that more closely matches what it is/is used for. Then introduces a newSubtopologyclass which includes basic metadata such as the topic group id and the NamedTopology that this subtopology belongs to, if any.Also adds an internal
NamedTaskIdclass to expose thenamedTopologyof a TaskId outside the package. I realized TaskId is part of the public API, so we can't just add a publicnamedTopology()getter method. I made thenamedTopologyfield protected and removed the getter/moved it to the NamedTaskId class.There are no actual logical changes or features in this PR, it's just a refactoring. Split these changes out to reduce the LOC in the main PRs.