junrao commented on code in PR #18997:
URL: https://github.com/apache/kafka/pull/18997#discussion_r1968646894


##########
metadata/src/test/java/org/apache/kafka/controller/QuorumControllerTest.java:
##########
@@ -373,7 +373,7 @@ public  void testElrEnabledByDefault() throws Throwable {
                 )).
                 build()
         ) {
-            controlEnv.activeController(true);
+            controlEnv.activeController();

Review Comment:
   Thinking a bit more. It seems that this test exposes a real bug introduced 
in 
https://github.com/apache/kafka/pull/16848/files#diff-77dc2adb187fd078084644613cff2b53021c8a5fbcdcfa116515734609d1332a.
   
   In the following code, we pick up the finalized feature version (including 
MV) first and pass them to the registerBroker event. It's possible that the 
passed in MV is outdated, but when the registerBroker event is processed, the 
MV becomes update to date. This will fail a registerBroker request that 
shouldn't be failed. It seems that we should pick up the finalized feature 
version when the registerBroker event is processed.
   ```
   public CompletableFuture<BrokerRegistrationReply> registerBroker(
       ControllerRequestContext context,
       BrokerRegistrationRequestData request
   ) {
       // populate finalized features map with latest known kraft version for 
validation
       Map<String, Short> controllerFeatures = new 
HashMap<>(featureControl.finalizedFeatures(Long.MAX_VALUE).featureMap());
       controllerFeatures.put(KRaftVersion.FEATURE_NAME, 
raftClient.kraftVersion().featureLevel());
       return appendWriteEvent("registerBroker", context.deadlineNs(),
           () -> clusterControl.
               registerBroker(request, offsetControl.nextWriteOffset(),
                   new FinalizedControllerFeatures(controllerFeatures, 
Long.MAX_VALUE),
                   context.requestHeader().requestApiVersion() >= 3),
           EnumSet.noneOf(ControllerOperationFlag.class));
   }
   ```
   
   Colin confirmed that this is an issue. In general, we should not be 
accessing featureControl.finalizedFeatures outside of the event queue thread 
since it introduces potential concurrency issues.



-- 
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-unsubscr...@kafka.apache.org

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

Reply via email to