You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cadonna (via GitHub)" <gi...@apache.org> on 2023/04/04 08:32:12 UTC

[GitHub] [kafka] cadonna commented on a diff in pull request #13498: MINOR: fix log statement

cadonna commented on code in PR #13498:
URL: https://github.com/apache/kafka/pull/13498#discussion_r1156908192


##########
streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java:
##########
@@ -1536,7 +1536,7 @@ private Consumer<StreamThread> streamThreadLeaveConsumerGroup(final long remaini
                     removeMembersFromConsumerGroupResult.memberResult(memberToRemove)
                         .get(remainingTimeMs, TimeUnit.MILLISECONDS);
                 } catch (final Exception e) {
-                    log.error("Could not remove static member {} from consumer group {} due to a: {}",
+                    log.error("Could not remove static member {} from consumer group {} due to:",
                         groupInstanceId.get(),
                         applicationConfigs.getString(StreamsConfig.APPLICATION_ID_CONFIG), e);

Review Comment:
   I guess you want to log the exception, right? If yes, a third pair of curly braces is missing, isn't it? Method `error()` has signatures
   ```
   public void error(String format, Object... arguments);
   ```
   and 
   ```
   public void error(String msg, Throwable t);
   ```
   There is no signature that takes a number of arguments and then a `Throwable`. I pretty sure that would be incorrect java.
   
   My suggestion is:
   ```suggestion
                       log.error("Could not remove static member {} from consumer group {} due to: {}",
                           groupInstanceId.get(),
                           applicationConfigs.getString(StreamsConfig.APPLICATION_ID_CONFIG), 
                           e
                       );
   ```
   or
   ```suggestion
                       log.error("Could not remove static member {} from consumer group {} due to: {}",
                           groupInstanceId.get(), applicationConfigs.getString(StreamsConfig.APPLICATION_ID_CONFIG), e);
   ```
   
   Logging the stacktrace would also be great!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org