[PR] KAFKA-18599: Remove Optional wrapping for forwardingManager in ApiVersionManager [kafka]

["mingyen066 (via GitHub)" <gi...@apache.org>]

mingyen066 opened a new pull request, #18630:
URL: https://github.com/apache/kafka/pull/18630

   Since we can always get forwardingManager now, unwrap the Optional for the forwardingManager field in ApiVersionManager.
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


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

Re: [PR] KAFKA-18599: Remove Optional wrapping for forwardingManager in ApiVersionManager [kafka]

["ijuma (via GitHub)" <gi...@apache.org>]

ijuma merged PR #18630:
URL: https://github.com/apache/kafka/pull/18630


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

Re: [PR] KAFKA-18599: Remove Optional wrapping for forwardingManager in ApiVersionManager [kafka]

["ijuma (via GitHub)" <gi...@apache.org>]

ijuma commented on code in PR #18630:
URL: https://github.com/apache/kafka/pull/18630#discussion_r1924609967


##########
core/src/test/scala/unit/kafka/server/ApiVersionManagerTest.scala:
##########
@@ -103,51 +105,29 @@ class ApiVersionManagerTest {
     val forwardingManager = Mockito.mock(classOf[ForwardingManager])
     Mockito.when(forwardingManager.controllerApiVersions).thenReturn(None)
 
-    for (forwardingManagerOpt <- Seq(Some(forwardingManager), None)) {
-      val versionManager = new DefaultApiVersionManager(
-        listenerType = ListenerType.BROKER,
-        forwardingManager = forwardingManagerOpt,
-        brokerFeatures = brokerFeatures,
-        metadataCache = metadataCache,
-        enableUnstableLastVersion = true
-      )
-      assertFalse(versionManager.isApiEnabled(ApiKeys.ENVELOPE, ApiKeys.ENVELOPE.latestVersion))
-      assertFalse(versionManager.enabledApis.contains(ApiKeys.ENVELOPE))
-
-      val apiVersionsResponse = versionManager.apiVersionResponse(throttleTimeMs = 0, false)
-      val envelopeVersion = apiVersionsResponse.data.apiKeys.find(ApiKeys.ENVELOPE.id)
-      assertNull(envelopeVersion)
-    }
-  }
-
-  @Disabled("Enable after enable KIP-590 forwarding in KAFKA-12886")
-  @Test
-  def testEnvelopeEnabledWhenForwardingManagerPresent(): Unit = {
-    val forwardingManager = Mockito.mock(classOf[ForwardingManager])
-    Mockito.when(forwardingManager.controllerApiVersions).thenReturn(None)
-
     val versionManager = new DefaultApiVersionManager(
-      listenerType = ListenerType.ZK_BROKER,
-      forwardingManager = Some(forwardingManager),
+      listenerType = ListenerType.BROKER,
+      forwardingManager = forwardingManager,
       brokerFeatures = brokerFeatures,
       metadataCache = metadataCache,
       enableUnstableLastVersion = true
     )
-    assertTrue(versionManager.isApiEnabled(ApiKeys.ENVELOPE, ApiKeys.ENVELOPE.latestVersion))
-    assertTrue(versionManager.enabledApis.contains(ApiKeys.ENVELOPE))
+    assertFalse(versionManager.isApiEnabled(ApiKeys.ENVELOPE, ApiKeys.ENVELOPE.latestVersion))
+    assertFalse(versionManager.enabledApis.contains(ApiKeys.ENVELOPE))
 
     val apiVersionsResponse = versionManager.apiVersionResponse(throttleTimeMs = 0, false)
     val envelopeVersion = apiVersionsResponse.data.apiKeys.find(ApiKeys.ENVELOPE.id)
-    assertNotNull(envelopeVersion)
-    assertEquals(ApiKeys.ENVELOPE.oldestVersion, envelopeVersion.minVersion)
-    assertEquals(ApiKeys.ENVELOPE.latestVersion, envelopeVersion.maxVersion)
+    assertNull(envelopeVersion)
   }
 
   @Test
-  def testEnvelopeDisabledWhenForwardingManagerEmpty(): Unit = {
+  def testEnvelopeEnabledWhenForwardingManagerPresent(): Unit = {

Review Comment:
   We can delete this since `ZK_BROKER` is being deleted.



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