Add support to MaxStalenessSeconds in ReadPreference#271
Add support to MaxStalenessSeconds in ReadPreference#271wpjunior wants to merge 3 commits intoglobalsign:developmentfrom
Conversation
wpjunior
force-pushed
the
MaxStalenessSeconds
branch
4 times, most recently
from
268d296 to
ad7fb14
Compare
wpjunior
changed the title
domodwyer
left a comment
domodwyer
left a comment
There was a problem hiding this comment.
Is there any way to add functional tests for this, beyond the config parsing? Seems like there might be a few edge cases.
| } | ||
| case "maxStalenessSeconds": | ||
| maxStalenessSeconds, err = strconv.Atoi(opt.value) | ||
| if err != nil { |
There was a problem hiding this comment.
I think this would need the maxStalenessSeconds >= 0 check to compliment the SetMaxStalenessSeconds() behaviour?
|
|
||
| // SetMaxStalenessSeconds set the maximum of seconds of replication lag from secondaries | ||
| // | ||
| // Relevant documentation: |
There was a problem hiding this comment.
It might be worth including that if seconds > 90 it's currently expected to receive an error from mongodb:
You must specify a maxStalenessSeconds value of 90 seconds or longer: specifying a smaller maxStalenessSeconds value will raise an error.
| Mode Mode | ||
|
|
||
| // MaxStalenessSeconds specify a maximum replication lag, or “staleness” in seconds, for reads from secondaries. | ||
| MaxStalenessSeconds int |
There was a problem hiding this comment.
Is this missing an omitempty tag? I imagine you don't want to send maxStalenessSeconds: 0 with every request if it was not set by the user.
There was a problem hiding this comment.
hi @domodwyer, the wire information is sent by socket.go(line: 131), I suppose that tag is not necessary here, thanks.
| if err != nil { | ||
| return nil, errors.New("bad value for maxPoolSize: " + opt.value) | ||
| } | ||
| case "maxStalenessSeconds": |
There was a problem hiding this comment.
According to the documentation maxStalenessSeconds is not compatible with mode primary, so maybe you could add that restriction to the existing check below (line 460).
There was a problem hiding this comment.
Also, could you please add description of this parameter to the Dial method documentation (i.e. https://godoc.org/github.com/globalsign/mgo#Dial)
session.go
Outdated
| // | ||
| func (s *Session) SetMaxStalenessSeconds(seconds int) { | ||
| s.m.Lock() | ||
| if seconds > -1 { |
There was a problem hiding this comment.
The documentation states you must specify a maxStalenessSeconds value of 90 seconds or longer (smaller values will raise an error). Since you're doing a validity check here it might be worth checking that as well.
| // Mode determines the consistency of results. See Session.SetMode. | ||
| Mode Mode | ||
|
|
||
| // MaxStalenessSeconds specify a maximum replication lag, or “staleness” in seconds, for reads from secondaries. |
There was a problem hiding this comment.
Could you please add a comment stating that this option is supported in mongo >= 3.4 only ?
| if err != nil { | ||
| return nil, errors.New("bad value for maxPoolSize: " + opt.value) | ||
| } | ||
| case "maxStalenessSeconds": |
There was a problem hiding this comment.
Also, could you please add description of this parameter to the Dial method documentation (i.e. https://godoc.org/github.com/globalsign/mgo#Dial)
wpjunior
force-pushed
the
MaxStalenessSeconds
branch
4 times, most recently
from
67ff921 to
39ae9df
Compare
wpjunior
force-pushed
the
MaxStalenessSeconds
branch
from
39ae9df to
7bf893d
Compare
eminano
left a comment
eminano
left a comment
There was a problem hiding this comment.
As @domodwyer mentioned previously, is it possible to add functional tests for this?
|
Hi @eminano, would you check the possibility to approve this MR ?, this scenario is too hard to create an integration test. Thanks =) |
|
It seems the changes only pass a ReadPreference params to server and did not select proper servers, so at my guess the driver will not work with mongo replset but sharding cluster. |
6 participants
from #270