RUBY-1174 RUBY-1178 Update Max Staleness implementation #836
Conversation
| unless @max_staleness > 0 && | ||
| @max_staleness >= [ SMALLEST_MAX_STALENESS_SECONDS, | ||
| (heartbeat_frequency + Cluster::IDLE_WRITE_PERIOD_SECONDS) ].max | ||
| raise Error::InvalidServerPreference.new(Error::InvalidServerPreference::INVALID_MAX_STALENESS) |
There was a problem hiding this comment.
Consider giving more detail in this error. See https://github.com/mongodb/mongo-java-driver/blob/master/driver-core/src/main/com/mongodb/TaggableReadPreference.java#L196-L203 for example
| raise Error::InvalidServerPreference.new(Error::InvalidServerPreference::INVALID_MAX_STALENESS) | ||
| if @max_staleness | ||
| heartbeat_frequency = cluster.options[:heartbeat_frequency] || Server::Monitor::HEARTBEAT_FREQUENCY | ||
| unless @max_staleness > 0 && |
There was a problem hiding this comment.
The @max_staleness > 0 check is redundant, since the next check is checking that it's at least >= 90
| if @max_staleness < heartbeat_frequency * 2 | ||
| raise Error::InvalidServerPreference.new(Error::InvalidServerPreference::INVALID_MAX_STALENESS) | ||
| if @max_staleness | ||
| heartbeat_frequency = cluster.options[:heartbeat_frequency] || Server::Monitor::HEARTBEAT_FREQUENCY |
There was a problem hiding this comment.
Rename to heartbeat_frequency_seconds to clarify that it's the right time unit.
| validate_max_staleness_value!(cluster) | ||
| select(cluster.servers) | ||
| servers = cluster.servers | ||
| validate_max_staleness_value!(cluster) unless servers.empty? |
There was a problem hiding this comment.
Does cluster.servers include only the connected servers? If so, I'm not clear how ReplicaSetNoPrimary/NoKnownServers.yml is passing.
There was a problem hiding this comment.
ReplicaSetNoPrimary/NoKnownServers.yml was added after my day ended yesterday so was not in this PR.
I've just updated them to the latest, was failing ReplicaSetNoPrimary/NoKnownServers.yml but then changed the code to do the check unless Topology type is Unknown.
…ogy type is Unknown
|
@jyemin, I've addressed all comments. If you could review one more time, that'd be great. |
|
LGTM |
|
@jarthod Do you want to take a look at this as well to verify unit conversions? |
|
Looks good to me too ;) |
|
thanks @jarthod = ) |
3 participants
No description provided.