MINOR-restructuring: Relocated PartitionMetadataClient to server-common#21440
MINOR-restructuring: Relocated PartitionMetadataClient to server-common#21440apoorvmittal10 merged 4 commits intoapache:trunkfrom
Conversation
github-actions
bot
added
triage
| @@ -54,7 +54,7 @@ | |||
| import java.util.concurrent.atomic.AtomicBoolean; | |||
| import java.util.function.Supplier; | |||
|
|
|||
| public class NetworkPartitionMetadataClient implements PartitionMetadataClient { | |||
| public class NetworkPartitionMetadataClient implements org.apache.kafka.server.util.PartitionMetadataClient { | |||
There was a problem hiding this comment.
could we remove the fqcn in favor of just the class name?
| @@ -177,14 +177,14 @@ public void testListLatestOffsetsSuccess() throws ExecutionException, Interrupte | |||
| Set<TopicPartition> partitions = new HashSet<>(); | |||
| partitions.add(tp); | |||
|
|
|||
| Map<TopicPartition, CompletableFuture<PartitionMetadataClient.OffsetResponse>> futures = | |||
| Map<TopicPartition, CompletableFuture<org.apache.kafka.server.util.PartitionMetadataClient.OffsetResponse>> futures = | |||
| networkPartitionMetadataClient.listLatestOffsets(partitions); | ||
|
|
||
| assertNotNull(futures); | ||
| assertEquals(1, futures.size()); | ||
| assertTrue(futures.containsKey(tp)); | ||
|
|
||
| PartitionMetadataClient.OffsetResponse response = futures.get(tp).get(); | ||
| org.apache.kafka.server.util.PartitionMetadataClient.OffsetResponse response = futures.get(tp).get(); |
smjn
left a comment
smjn
left a comment
There was a problem hiding this comment.
Thanks for the PR, the NetworkPartitionMetadata seems general enough to be used by other coordinator runtime specializations as well so makes sense to move it to a general module.
Minor comments related to class name import and use.
smjn
added
KIP-932
smjn
left a comment
smjn
left a comment
There was a problem hiding this comment.
Thanks for the changes, LGTM
squah-confluent
left a comment
squah-confluent
left a comment
There was a problem hiding this comment.
Thanks for the patch!
| @@ -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.server; | |||
There was a problem hiding this comment.
Shall we also put this in org.apache.kafka.server.util?
There was a problem hiding this comment.
Thanks for the review. NetworkPartitionMetadataClient uses MetadataCache which is present in the metadata module. Since server-common is already a dependency of metadata, moving NetworkPartitionMetadataClient would create a cyclic dependency. That's why NetworkPartitionMetadataClient was moved to the server module instead.
There was a problem hiding this comment.
I meant the package within server, not the module.
There was a problem hiding this comment.
Ohh ok, my bad. I have made that change in the latest commit.
4 participants
This PR moves PartitionMetadataClient to server-common so it can be
reused across multiple components. The change improves modularity,
avoids duplication, and makes these clients accessible from all required
call sites without tight coupling.