kafka-jira mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (KAFKA-6024) Consider moving validation in KafkaConsumer ahead of call to acquireAndEnsureOpen()
Date Tue, 13 Mar 2018 06:04:00 GMT

    [ https://issues.apache.org/jira/browse/KAFKA-6024?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16396592#comment-16396592
] 

ASF GitHub Bot commented on KAFKA-6024:
---------------------------------------

hachikuji closed pull request #4617: KAFKA-6024 Move validation in KafkaConsumer ahead of
acquireAndEnsure…
URL: https://github.com/apache/kafka/pull/4617
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java b/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
index 3cd034eff76..81137f3c8dd 100644
--- a/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
+++ b/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java
@@ -966,13 +966,12 @@ public void subscribe(Collection<String> topics) {
      */
     @Override
     public void subscribe(Pattern pattern, ConsumerRebalanceListener listener) {
+        if (pattern == null)
+            throw new IllegalArgumentException("Topic pattern to subscribe to cannot be null");
+
         acquireAndEnsureOpen();
         try {
-            if (pattern == null)
-                throw new IllegalArgumentException("Topic pattern to subscribe to cannot
be null");
-
             throwIfNoAssignorsConfigured();
-
             log.debug("Subscribed to pattern: {}", pattern);
             this.subscriptions.subscribe(pattern, listener);
             this.metadata.needMetadataForAllTopics(true);
@@ -1337,11 +1336,11 @@ public void commitAsync(final Map<TopicPartition, OffsetAndMetadata>
offsets, Of
      */
     @Override
     public void seek(TopicPartition partition, long offset) {
+        if (offset < 0)
+            throw new IllegalArgumentException("seek offset must not be a negative number");
+
         acquireAndEnsureOpen();
         try {
-            if (offset < 0)
-                throw new IllegalArgumentException("seek offset must not be a negative number");
-
             log.debug("Seeking to offset {} for partition {}", offset, partition);
             this.subscriptions.seek(partition, offset);
         } finally {
@@ -1357,11 +1356,11 @@ public void seek(TopicPartition partition, long offset) {
      * @throws IllegalArgumentException if {@code partitions} is {@code null} or the provided
TopicPartition is not assigned to this consumer
      */
     public void seekToBeginning(Collection<TopicPartition> partitions) {
+        if (partitions == null)
+            throw new IllegalArgumentException("Partitions collection cannot be null");
+
         acquireAndEnsureOpen();
         try {
-            if (partitions == null) {
-                throw new IllegalArgumentException("Partitions collection cannot be null");
-            }
             Collection<TopicPartition> parts = partitions.size() == 0 ? this.subscriptions.assignedPartitions()
: partitions;
             for (TopicPartition tp : parts) {
                 log.debug("Seeking to beginning of partition {}", tp);
@@ -1383,11 +1382,11 @@ public void seekToBeginning(Collection<TopicPartition> partitions)
{
      * @throws IllegalArgumentException if {@code partitions} is {@code null} or the provided
TopicPartition is not assigned to this consumer
      */
     public void seekToEnd(Collection<TopicPartition> partitions) {
+        if (partitions == null)
+            throw new IllegalArgumentException("Partitions collection cannot be null");
+
         acquireAndEnsureOpen();
         try {
-            if (partitions == null) {
-                throw new IllegalArgumentException("Partitions collection cannot be null");
-            }
             Collection<TopicPartition> parts = partitions.size() == 0 ? this.subscriptions.assignedPartitions()
: partitions;
             for (TopicPartition tp : parts) {
                 log.debug("Seeking to end of partition {}", tp);


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Consider moving validation in KafkaConsumer ahead of call to acquireAndEnsureOpen()
> -----------------------------------------------------------------------------------
>
>                 Key: KAFKA-6024
>                 URL: https://issues.apache.org/jira/browse/KAFKA-6024
>             Project: Kafka
>          Issue Type: Improvement
>            Reporter: Ted Yu
>            Assignee: siva santhalingam
>            Priority: Minor
>             Fix For: 1.2.0
>
>
> In several methods, parameter validation is done after calling acquireAndEnsureOpen()
:
> {code}
>     public void seek(TopicPartition partition, long offset) {
>         acquireAndEnsureOpen();
>         try {
>             if (offset < 0)
>                 throw new IllegalArgumentException("seek offset must not be a negative
number");
> {code}
> Since the value of parameter would not change per invocation, it seems performing validation
ahead of acquireAndEnsureOpen() call would be better.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message