Clients v2 #316
Clients v2 #316
Conversation
Codecov Report
@@ Coverage Diff @@
## master #316 +/- ##
==========================================
- Coverage 74.39% 72.71% -1.69%
==========================================
Files 65 71 +6
Lines 3691 3859 +168
==========================================
+ Hits 2746 2806 +60
- Misses 839 939 +100
- Partials 106 114 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
looks good overall, just it might be better to split per client (like in #315 ) ?
So that also the review is more structured and easier making it in smaller pieces.
But also happy to go with this bigger one ... given the circumstances and work put into this.
| // Close shuts down the producer and waits for any buffered messages to be | ||
| // flushed. You must call this function before a producer object passes out of | ||
| // scope, as it may otherwise leak memory. | ||
| func (ap *AsyncProducer) Close() error { |
There was a problem hiding this comment.
maybe we could use defer in this case to make the code more clear
(and return a potential error with a pattern like this pattern like this https://yourbasic.org/golang/defer/#use-func-to-return-a-value ) ?
There was a problem hiding this comment.
we might have potentially 2 errors here. the aggregate function merges whose 2 into one. with the mentioned pattern we will only return 1 error which is not what we want here.
|
|
||
| prometheus.MustRegister(messageStatus) | ||
| } | ||
|
|
There was a problem hiding this comment.
are status and deliveryType values limited to a specific number ?
Maybe we could use enums or predefined string properties ?
There was a problem hiding this comment.
i had to revert one of the values as it broke the tests. the reason is that they are internals and integration tests do not run inside the same package.
| // Close shuts down the producer and waits for any buffered messages to be | ||
| // flushed. You must call this function before a producer object passes out of | ||
| // scope, as it may otherwise leak memory. | ||
| func (p *SyncProducer) Close() error { |
| @@ -5,7 +5,7 @@ import ( | |||
| "io" | |||
| "io/ioutil" | |||
|
|
|||
| "github.com/golang/protobuf/proto" | |||
| "github.com/golang/protobuf/proto" //nolint:staticcheck | |||
There was a problem hiding this comment.
it might be a better option to use the golang.org package instead of ignoring the error ?
There was a problem hiding this comment.
we have an open ticket for this. Unfortunately it is not straightforward with just an upgrade because it is a breaking change.
Which problem is this PR solving?
Closes #311.
Short description of the changes