Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Outputhost: Fix offset computations for ack/read levels #193

Merged
merged 3 commits into from
May 8, 2017

Conversation

kirg
Copy link
Contributor

@kirg kirg commented May 5, 2017

No description provided.

@kirg kirg self-assigned this May 5, 2017
@kirg kirg requested a review from aravindvs May 5, 2017 22:56
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 68.078% when pulling 18cc8c0 on fix-offsets-bug into dcb4715 on master.

@kirg kirg requested a review from GuillaumeBailey May 5, 2017 23:39
Copy link
Contributor

@aravindvs aravindvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.. I have a couple of minor comments.. feel free to get it in once those are addressed..

}

levels struct {
asOf common.UnixNanoTime // Timestamp when this level was calculated
readLevel common.SequenceNumber // -1 = nothing received, 0 = 1 message (#0) received, etc.
ackLevel common.SequenceNumber // -1 = nothing acked
readLevel ackIndex // -1 = nothing received, 0 = 1 message (#0) received, etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this comment? I don't think we initialize to -1 now..

ackMgr.logger.WithFields(bark.Fields{
`old-readLevelSeq`: ackMgr.readLevelSeq,
`new-readLevelSeq`: sequence,
}).Error(`adjusting read-level sequence`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a Warn level log and let's also record a metric here for easy tracking..

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 67.851% when pulling fde78f9 on fix-offsets-bug into dcb4715 on master.

@kirg kirg merged commit a96faf0 into master May 8, 2017
@kirg kirg deleted the fix-offsets-bug branch May 8, 2017 04:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants