[GitHub] [kafka] C0urante commented on a change in pull request #10014: KAFKA-12252 and KAFKA-12262: Fix key rotation when leadership changes

[GitBox <gi...@apache.org>]

C0urante commented on a change in pull request #10014:
URL: https://github.com/apache/kafka/pull/10014#discussion_r568653512



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java
##########
@@ -381,7 +381,7 @@ public void tick() {
             log.debug("Scheduled rebalance at: {} (now: {} nextRequestTimeoutMs: {}) ",
                     scheduledRebalance, now, nextRequestTimeoutMs);
         }
-        if (internalRequestValidationEnabled() && keyExpiration < Long.MAX_VALUE) {
+        if (isLeader() && internalRequestValidationEnabled() && keyExpiration < Long.MAX_VALUE) {

Review comment:
       There's a rationale for leaving the check in here but I'm not sold either way. Let me know what you think:
   
   The `Scheduled next key rotation at...` log message should ideally only be emitted if key rotation is actually enabled. Key rotation can be [disabled by setting the ttl to 0](https://github.com/apache/kafka/blob/821d867c1bd01b88aa695827cb57c9aa764857a4/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L1579). If someone has key rotation disabled, emitting this log message doesn't make a lot of sense (even if the scheduled rotation time is millions of years in the future).
   
   This also mirrors the structure of the scheduled rebalance delay check above, although that check is more necessary because of the tweaking of the `rebalanceResolved` field.




----------------------------------------------------------------
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.

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