feat: message ack sync changes improvements #43
Conversation
Removed the connection block on reading the messages for open connection Implemented the single go-routine for the sync ack reporting Moved the ack changes to the new file ack.go Added few metrics to check the tt for kafka batch publish and for the ack.
AkbaraliShaikh
requested review from
chakravarthyvp,
NNcrawler,
ramey and
ravisuhag
|
|
||
| var AckChan = make(chan AckInfo) | ||
|
|
||
| type AckInfo struct { |
There was a problem hiding this comment.
@AkbaraliShaikh If package itself is ack. This can be named just Info to remove brevity in usage. ack.AckInfo
There was a problem hiding this comment.
The package name is not ack so the name is used as AckInfo. It's not a sub package.
| AckTimeConsumed time.Time | ||
| } | ||
|
|
||
| func AckHandler(ch <-chan AckInfo) { |
There was a problem hiding this comment.
Same as above. Can it be just Handler?
There was a problem hiding this comment.
We cannot, It's not under the ack package.
|
|
||
| tim := time.Since(c.TimeConsumed) | ||
| if c.Err != nil { | ||
| metrics.Timing("event_rtt_ms", tim.Milliseconds(), "") |
There was a problem hiding this comment.
@AkbaraliShaikh What is the diff between ack_event_rtt_ms and event_rtt_ms ? It will be confusing if both are different ones. Also let's have a consistent naming of metrics.
There was a problem hiding this comment.
event_rtt_ms -> is used to track the total time taken for the event (from consume -> response)
ack_event_rtt_ms -> is used to track the total time taken for only acknowledging (time spent in the ack channel).
Removed the connection block on reading the messages for open active connection
Implemented the single go-routine for the sync ack response reporting
Moved the ack changes to the new file ack.go
Added couple of new metrics to check the tt for kafka batch publish and for the ack response.
#42
Perf Results:
Ack Channelgo-routine per connection (blue) vs single go-routine for all connections (yellow)