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

[8.x](backport #41824) Fix Kafka output panic at startup #41828

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 28, 2024

In #40794 the Connectable interface was changed to include a context.Context argument, causing all clients with a Connect() that weren't updated to have a Context(context.Context) method instead to fail the NetworkClient interface check in

if nc, ok := client.(outputs.NetworkClient); ok {
c = &netClientWorker{
worker: w,
client: nc,
logger: logger,
tracer: tracer,
}

This made it so that the Kafka and Redis outputs no longer counted as NetworkClients, bypassing the initial call to Connect() that was there previously at

err := w.client.Connect(ctx)

In the case of Kafka the Connect call never happening makes the output panic on the first call to Publish because there is no producer.

Raising without tests to make sure nothing else was missed while I figure out the best test for this that isn't totally contrived, because the problem is in the publisher pipeline and the output specific tests don't hook in at that level. This is how the problem was originally missed.

The PR has been updated with an integration test to reproduce the problem: #41824 (comment)


This is an automatic backport of pull request #41824 done by [Mergify](https://mergify.com).

* Make Kafka output satisfy NetworkClient interface.

* Make Redis output satisfy network client.

* Add initial regression integration test.

* Add an integration test to ensure connectivity.

* Fix build error in old integration test.

* Fix redis lint error.

* Fix typo in comment.

* Fix another typo.

(cherry picked from commit e42589d)
@mergify mergify bot added the backport label Nov 28, 2024
@mergify mergify bot requested a review from a team as a code owner November 28, 2024 23:32
@mergify mergify bot removed the request for review from a team November 28, 2024 23:32
@mergify mergify bot requested review from mauri870 and faec November 28, 2024 23:32
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 28, 2024
@botelastic
Copy link

botelastic bot commented Nov 28, 2024

This pull request doesn't have a Team:<team> label.

@cmacknz cmacknz enabled auto-merge (squash) November 29, 2024 01:09
@cmacknz cmacknz merged commit 47636d9 into 8.x Nov 29, 2024
139 of 141 checks passed
@cmacknz cmacknz deleted the mergify/bp/8.x/pr-41824 branch November 29, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant