Average RTT Calculation & Tests #2
Conversation
| Connection() (Connection, error) | ||
| } | ||
|
|
||
| const UnsetRTT = -1 * time.Millisecond |
There was a problem hiding this comment.
Is there ever a time when the delay will be 0 nanoseconds? Can we just let 0 mean unset? That way we don't have to remember to set it to UnsetRTT everywhere?
Also, does this need to be exported?
There was a problem hiding this comment.
that's what I had originally started with (because in real life there would be no way that should happen), but the rtt tests actually require the 0 is treated as a valid delay value distinct from 'NULL', which is why I made this change.
As far as exporting, I just exported because the AverageRTT() getter was exported, figuring that the two sort of go together.
core/server_monitor.go
Outdated
|
|
||
| desc = buildServerDesc(m.connectionOpts.Endpoint, isMasterResult, buildInfoResult) | ||
| desc.averageRTT = delay // TODO: actually calculate this properly | ||
| desc.updateAverageRTT(delay) |
There was a problem hiding this comment.
desc here is brand new, so it's RTT will always be unset. You need to get this value from somewhere else... perhaps store it in the ServerMonitor itself. I don't think you'll need a lock for it since this method will never happen concurrently.
|
Updated the patch to persist the averageRTT value in the ServerMonitor |
There was a problem hiding this comment.
So, in addition to the inline comments, I think ServerDesc should be changed as follows such that we don't have to remember to set averageRTT all the time. First, add a field (averageRTTSet bool).
Second, have the AverageRTT() function return UnsetRTT when averageRTTSet is false, and return averageRTT otherwise. That way our zero value is still correct.
core/server_monitor.go
Outdated
|
|
||
| desc = buildServerDesc(m.connectionOpts.Endpoint, isMasterResult, buildInfoResult) | ||
| desc.updateAverageRTT(delay) | ||
| desc.updateAverageRTT(m.averageRTT, delay) |
There was a problem hiding this comment.
if you can read it here, just set it back here, where it gets calculated... no need to set it in a different place.
core/server.go
Outdated
| // given its most recent RTT value | ||
| func (d *ServerDesc) updateAverageRTT(delay time.Duration) { | ||
| if d.averageRTT == UnsetRTT { | ||
| func (d *ServerDesc) updateAverageRTT(averageRTT, delay time.Duration) { |
There was a problem hiding this comment.
perhaps pull this function out and put it somewhere else. At this point, there isn't any reason for it to be hanging off ServerDesc.
core/spec_rtt_internal_test.go
Outdated
| averageRTT: startingAverageRTT, | ||
| } | ||
| desc.updateAverageRTT(newRTT) | ||
| desc := ServerDesc{} |
There was a problem hiding this comment.
I don't think the tests need to care about ServerDesc at all. How ServerDesc carries around the RTT value I think is of little concern to getting this calculation correct.
|
|
||
| // updateAverageRTT calcuates the averageRTT of the server | ||
| // given its most recent RTT value | ||
| func (m *ServerMonitor) updateAverageRTT(delay time.Duration) time.Duration { |
There was a problem hiding this comment.
decided to make this a method on ServerMonitor, since that is where we are actually persisting the RTT
| return d.version | ||
| } | ||
|
|
||
| func (d *ServerDesc) setAverageRTT(rtt time.Duration) { |
There was a problem hiding this comment.
just a helper here so we don't have to manually set averageRTTSet = true
|
I went with your suggestion of a flag so that the zero values of all fields represent the unset state. Also moved the updateAverageRTT function to the monitor, because it seemed more appropriate there. |
# This is the 1st commit message: test # This is the commit message mongodb#2: test2
2 participants
No description provided.