Add support to MaxStalenessSeconds in ReadPreference #271
Conversation
wpjunior
force-pushed
the
MaxStalenessSeconds
branch
4 times, most recently
from
268d296
to
ad7fb14
Compare
wpjunior
changed the title
| @@ -390,6 +391,11 @@ func ParseURL(url string) (*DialInfo, error) { | |||
| if err != nil { | |||
| return nil, errors.New("bad value for maxPoolSize: " + opt.value) | |||
| } | |||
| 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?
There was a problem hiding this comment.
thanks @domodwyer, I've added you recommendation.
| @@ -2190,6 +2204,22 @@ func (s *Session) SetPoolTimeout(timeout time.Duration) { | |||
| s.m.Unlock() | |||
| } | |||
|
|
|||
| // 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.
| @@ -679,6 +689,9 @@ type ReadPreference struct { | |||
| // 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. | |||
| 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.
| @@ -390,6 +391,11 @@ func ParseURL(url string) (*DialInfo, error) { | |||
| 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.
| @@ -679,6 +689,9 @@ type ReadPreference struct { | |||
| // 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 ?
| @@ -390,6 +391,11 @@ func ParseURL(url string) (*DialInfo, error) { | |||
| 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
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. |
from #270