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

feat(kafka): add producer config capabilities for connections #3371

Merged
merged 27 commits into from
Mar 21, 2024

Conversation

sicoyle
Copy link
Contributor

@sicoyle sicoyle commented Mar 1, 2024

Description

For the sidecars using the existing kafka client configurations, after a connection has been closed by the client after the default connection idle timeout of 9 minutes, then if that sidecar goes to write and publish a message they will see a write: broken pipe err. This is due to the connection being deemed idle, but the sidecar has no idea, and so when it tries to publish, then it is doing so to a closed connection to which we see that error. It is quite confusing for end users, and so these metadata fields allow us to keep a connection alive and establish a refresh for the topic metadata such that the connection may be kept alive indefinitely if desired.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

@sicoyle sicoyle requested review from a team as code owners March 1, 2024 16:02
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 1, 2024

I can update docs after I get initial feedback here pls :)

Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 1, 2024

Looks like main still has rabbit mq issues based on these test results

common/component/kafka/metadata.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 5, 2024

unrelated failure due to stable components certification test for bindings.rabbitmq and pubsub.rabbitmq

@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 5, 2024

ready!

common/component/kafka/metadata.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 6, 2024

this is now a dependency: dapr/kit#88

Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
Signed-off-by: Samantha Coyle <[email protected]>
@olitomlinson
Copy link

I've had a couple of issues with broken pipe so this fix will be well received :)

@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 12, 2024

ready

@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 12, 2024

ping @ItalyPaleAle ready :)

@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 15, 2024

ping for review please

@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 18, 2024

ready for review please

bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
@yaron2
Copy link
Member

yaron2 commented Mar 20, 2024

@sicoyle please see latest comments from @JoshVanL. We should be good to merge once addressed

@sicoyle sicoyle requested a review from JoshVanL March 20, 2024 14:12
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 20, 2024

addressed feedback. Thanks @JoshVanL

bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
Signed-off-by: Samantha Coyle <[email protected]>
@sicoyle
Copy link
Contributor Author

sicoyle commented Mar 21, 2024

ready @ItalyPaleAle Thanks for the extra feedback!

@ItalyPaleAle ItalyPaleAle merged commit 85252be into dapr:main Mar 21, 2024
86 of 89 checks passed
@sicoyle sicoyle deleted the feat-kafka-producer-configs branch March 21, 2024 16:41
@ItalyPaleAle ItalyPaleAle added this to the v1.14 milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants