Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Conversation

@TBSliver
Copy link
Contributor

@TBSliver TBSliver commented Jan 5, 2018

Includes a skipped test spec that I am unsure if applies to this driver, with the PossiblePrimary option.

Copy link
Contributor

@xdg xdg left a comment

Choose a reason for hiding this comment

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

I think this logic has turned out (a) more complex than it needs to be and (b) possibly incorrect as a result.

For one, by not checking all server descriptions for a minimum (or null), the value becomes a one-way ratchet.

I also find having both "defined_ls_timeout_minutes" and "logical_session_timeout_minutes" to add a lot of complexity.

I suggest having a single "logical_session_timeout_minutes" attribute of type Num and assign it -1 (as a signaling value) iff the ismaster response has no logicalSessionTimeoutMinutes value.

I also suggest adding a "is_data_bearing" method on Server that returns true for Standalone, RSPrimary, RSSecondary and Mongos types.

Then, the "update topology logical session timeout minutes" routine becomes relatively straightforward and easy to understand:

@data_bearing_servers = grep { $_->is_data_bearing } @all_servers;
$new_ls_timeout = @data_bearing_servers
    ? ( min map { $_->logical_session_timeout_minutes } @data_bearing_servers  )
    : -1;

(It might be slightly more complicated, but I think that's enough to give you the conceptual gist of it.)

Topology would also have to use "-1" as a signal value.

@xdg
Copy link
Contributor

xdg commented Jan 17, 2018

Updated my comment with a better example.

@TBSliver
Copy link
Contributor Author

Right have updated the logic - I think I may have misunderstood part of the spec originally.

Note that instead of using a -1 for a signalling value, I've used undef - as this is the value given in the spec, and used in the spec json files, so saves reworking all of those.

I've also added an is_data_bearing boolean to the server as suggested :D

@coveralls
Copy link

coveralls commented Jan 25, 2018

Coverage Status

Coverage decreased (-1.09%) to 82.741% when pulling e2f10f8 on shadow-dot-cat:TBSliver/PERL-799 into e477f53 on mongodb:master.

Copy link
Contributor

@xdg xdg left a comment

Choose a reason for hiding this comment

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

Looks good with one minor request for a comment

? $_->logical_session_timeout_minutes
: -1
} @data_bearing_servers;
if ( defined $timeout && $timeout < 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think $timeout would only be undef here if there were no data-bearing servers, right? If that's right, please add a comment to that effect for clarity.

@xdg
Copy link
Contributor

xdg commented Jan 25, 2018

Awesome! Rebased, squashed and merged!

@xdg xdg closed this Jan 25, 2018
@TBSliver TBSliver deleted the TBSliver/PERL-799 branch January 26, 2018 15:41
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