-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add kafka client pool #3493
Add kafka client pool #3493
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
Skipping CI for Draft Pull Request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3493 +/- ##
=============================================
- Coverage 62.18% 48.41% -13.77%
=============================================
Files 188 243 +55
Lines 12690 18017 +5327
Branches 273 0 -273
=============================================
+ Hits 7891 8723 +832
- Misses 4187 8560 +4373
- Partials 612 734 +122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
… properly enforced Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
…es to use the clientpool Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
/hold |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
/retest-required |
1 similar comment
/retest-required |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/cc @pierDipi |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
// guard against panic, this makes it easier to defer client.Close() on the caller side | ||
if c == nil { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this case happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I've fixed this now, we can probably remove it. Basically, if you do something like:
client, err := r.GetKafkaClient(...)
defer client.Close()
if err != nil {
...
}
Here, if err
is not nil
, the deferred client.Close()
method panics.
I fixed all the instances where this was happening, so I don't think we actually need this now
control-plane/pkg/prober/cache.go
Outdated
// Keys returns all of the keys currently in the cache that have not expired | ||
Keys() []K |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused, why do we need this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this was from an earlier version of the PR. Let me remove it
Signed-off-by: Calum Murray <cmurray@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest-required |
bd4ecfa
into
knative-extensions:main
Fixes #3397 , #3139
Proposed Changes
Release Note
Docs