Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-18274: Failed to restart controller in testing due to closed socket channel [1/2] #18310

Conversation

peterxcli
Copy link
Contributor

We should rebuild the metrics of SharedServer when restart the controller in test to prevent NPE.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added triage PRs from the community core Kafka Broker small Small PRs labels Dec 24, 2024
@@ -155,6 +156,11 @@ class ControllerServer(
() => featuresPublisher.features()
)

// metrics will be set to null when closing a controller, so we should recreate it for testing
if (sharedServer.metrics == null){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do similar fix to BrokerServer? It uses the metrics as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @chia7712 Thanks for your review.

Regarding the BrokerServer, it calls sharedServer.startForBroker() at the beginning of the startup method, which also checks if the metrics are null

However, we cannot apply the same approach to the ControllerServer because sharedServer.startForController() in it depends on listenerInfo. Therefore, this change is necessary.

Here is the relevant reference:

@peterxcli
Copy link
Contributor Author

Unrelated tests failed🥲:

Java23:

SaslSslAdminIntegrationTest "testCreateTopicsResponseMetadataAndConfig(String).quorum=kraft" org.opentest4j.AssertionFailedError: Expected java.util.concurrent.ExecutionException to be thrown, but nothing was thrown. 7.02s
PlaintextAdminIntegrationTest testConsumerGroupsDeprecatedConsumerGroupState(String, String).quorum=kraft.groupProtocol=consumer org.opentest4j.AssertionFailedError: Expected the offset for partition 0 to eventually become 1. 21.58s
PlaintextAdminIntegrationTest testConsumerGroups(String, String).quorum=kraft.groupProtocol=consumer org.opentest4j.AssertionFailedError: Expected the offset for partition 0 to eventually become 1. 22.18s

Java17:

AbstractCoordinatorTest testWakeupAfterSyncGroupReceivedExternalCompletion() org.opentest4j.AssertionFailedError: Should have woken up from ensureActiveGroup() ==> Expected org.apache.kafka.common.errors.WakeupException to be thrown, but nothing was thrown. 0.48s

@github-actions github-actions bot removed the triage PRs from the community label Dec 25, 2024
@chia7712 chia7712 merged commit c7c1364 into apache:trunk Dec 25, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants