Refactoring to reduce boilerplate #890

kirktrue · 2023-04-19T16:40:04Z

Overview:

Added convenience classes and methods to reduce boilerplate and ensure that the logic between KafkaConsumer and PrototypeAsyncConsumer matches as closely as possible
Changed call sites to use the above

kirktrue · 2023-04-19T18:14:13Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerUtils.java

+        return (List<ConsumerInterceptor<K, V>>) ClientUtils.interceptors(config, ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, ConsumerInterceptor.class);
+    }
+
+    final static class PartitionComparator implements Comparator<TopicPartition>, Serializable {


Reviewer's note: these Comparator classes were in an internal class named Utils. I renamed Utils to ConsumerUtils and added the utility methods.

kirktrue · 2023-04-19T18:14:42Z

...nts/src/main/java/org/apache/kafka/clients/consumer/internals/CoordinatorRequestManager.java

@@ -57,13 +57,11 @@ public class CoordinatorRequestManager implements RequestManager {
    private long totalDisconnectedMin = 0;
    private Node coordinator;

-    public CoordinatorRequestManager(


Reviewer's note: formatting.

kirktrue · 2023-04-19T18:15:25Z

clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java

@@ -881,18 +832,6 @@ public KafkaConsumer(Map<String, Object> configs,
        this.kafkaConsumerMetrics = new KafkaConsumerMetrics(metrics, "consumer");
    }

-    private static Metrics buildMetrics(ConsumerConfig config, Time time, String clientId) {


Reviewer's note: moved buildMetrics to ConsumerUtils.metrics

kirktrue · 2023-04-19T18:15:50Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java

@@ -62,13 +62,12 @@ public class CommitRequestManager implements RequestManager {
    private final boolean throwOnFetchStableOffsetUnsupported;
    final PendingRequests pendingRequests;

-    public CommitRequestManager(


Reviewer's note: formatting.

kirktrue · 2023-04-19T18:16:55Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThread.java

    }

    @Override
    public void run() {
+        running = true;


Reviewer's note: moved setting running to true from the constructor to the run method.

kirktrue · 2023-04-19T18:17:24Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThread.java

@@ -178,46 +169,34 @@ public void run() {
     * 3. Poll the networkClient to send and retrieve the response.
     */
    void runOnce() {
-        drain();


Reviewer's note: consolidated a lot of small methods into the runOnce method for clarity.

kirktrue · 2023-04-19T18:18:13Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/DefaultEventHandler.java

@@ -179,27 +147,6 @@ public boolean add(final ApplicationEvent event) {
        return applicationEventQueue.add(event);
    }

-    // bootstrap a metadata object with the bootstrap server IP address,


Reviewer's note: moved this code to the constructor and it's now using the utility classes/methods for less verbosity.

kirktrue · 2023-04-19T18:18:55Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/Deserializers.java

+    }
+
+    @SuppressWarnings("unchecked")
+    public Deserializers(ConsumerConfig config, Deserializer<K> keyDeserializer, Deserializer<V> valueDeserializer) {


Reviewer's note: this code came from KafkaConsumer's constructor. I moved it here to use it in the new consumer too.

kirktrue · 2023-04-19T18:19:35Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/NetworkClientDelegate.java

@@ -202,16 +202,13 @@ public static class UnsentRequest {
        private Optional<Node> node; // empty if random node can be choosen
        private Timer timer;

-        public UnsentRequest(


Reviewer's note: formatting.

kirktrue · 2023-04-19T18:20:07Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java

@@ -546,19 +524,6 @@ public ConsumerRecords<K, V> poll(long timeout) {
        throw new KafkaException("method not implemented");
    }

-    private static <K, V> ClusterResourceListeners configureClusterResourceListeners(


Reviewer's note: moved this to the ClientUtils class.

kirktrue · 2023-04-19T18:20:25Z

clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java

@@ -576,23 +541,6 @@ else if (newConfigs.get(VALUE_DESERIALIZER_CLASS_CONFIG) == null)
        return newConfigs;
    }

-    private static Metrics buildMetrics(


Reviewer's note: this is also now in the ClientUtils class.

clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestManagers.java

kirktrue · 2023-04-19T18:24:30Z

...s/src/test/java/org/apache/kafka/clients/consumer/internals/DefaultBackgroundThreadTest.java

        DefaultBackgroundThread backgroundThread = mockBackgroundThread();
        backgroundThread.start();
+


Reviewer's note: we need to inject a small wait here since I moved the running flag from the constructor to the run method.

clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java

kirktrue · 2023-04-20T23:58:24Z

There's a lint check error in some code that I didn't touch (ApiMessageType) that's complaining about non-serializable instance variables in a serializable outer class. I'll have to look into that more to get a clean run.

cla-assistant · 2023-10-12T15:57:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

kirktrue force-pushed the ctr-conveniences branch from f4a2b69 to eef91c2 Compare April 19, 2023 18:09

kirktrue marked this pull request as ready for review April 19, 2023 18:11

kirktrue requested review from a team as code owners April 19, 2023 18:11

kirktrue requested review from philipnee and lianetm April 19, 2023 18:11

kirktrue changed the base branch from master to ctr-staging April 19, 2023 18:11

kirktrue commented Apr 19, 2023

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/consumer/internals/RequestManagers.java Show resolved Hide resolved

kirktrue commented Apr 19, 2023

View reviewed changes

lianetm reviewed Apr 19, 2023

View reviewed changes

clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java Outdated Show resolved Hide resolved

kirktrue force-pushed the ctr-conveniences branch 2 times, most recently from 00bf9dd to e3dfa9a Compare April 24, 2023 22:06

Retrying the import

d0689a1

kirktrue force-pushed the ctr-conveniences branch from e3dfa9a to d0689a1 Compare April 24, 2023 22:11

Delete Utils.java

afd9cb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring to reduce boilerplate #890

Refactoring to reduce boilerplate #890

kirktrue commented Apr 19, 2023 •

edited

Loading

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue Apr 19, 2023

kirktrue commented Apr 20, 2023

cla-assistant bot commented Oct 12, 2023

		DefaultBackgroundThread backgroundThread = mockBackgroundThread();
		backgroundThread.start();

Refactoring to reduce boilerplate #890

Are you sure you want to change the base?

Refactoring to reduce boilerplate #890

Conversation

kirktrue commented Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirktrue commented Apr 20, 2023

cla-assistant bot commented Oct 12, 2023

kirktrue commented Apr 19, 2023 •

edited

Loading