Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPLIB-651: Support $merge and $out executing on secondaries #861

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Sep 9, 2021

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM pending approval of mongodb/specifications#1062


$server = select_server($this->manager, $options);
if ($server->isSecondary()) {
$server = select_server($this->manager, $options);
Copy link
Member Author

Choose a reason for hiding this comment

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

When I recently spoke to @kevinAlbs about this (re: mongodb/specifications#1062 (comment)), I mentioned an alternative implementation that could bypass a second server selection attempt (i.e. mongoc_client_select_server) by instead iterating on Manager::getServers() to select a primary.

Thanks to the first server selection attempt, we can be guaranteed that the topology has been discovered. getServers() shouldn't return an empty array unless there really are no servers available. But I also don't think there's much benefit to that approach vs. simply invoking server selection a second time. The edge case is that if there really isn't a primary available, we'll block for serverSelectionTimeoutMS on this second attempt.

But if the root concern is potentially blocking the application for 2 * serverSelectionTimeoutMS, let's consider how that might actually happen:

  • If the first attempt is looking for a secondary to execute an $out/$merge pipeline and the topology doesn't have a secondary at all, the default scenario will abort early thanks to serverSelectionTryOnce and abort the entire aggregate() method.
  • If serverSelectionTryOnce=false and a loop is permitted, server selection could theoretically discover a secondary just before serverSelectionTimeoutMS expires (e.g. at 29 seconds in) and return that server.
  • If we then discover the selected server is a pre-5.0 secondary, we'll re-enter server selection to look for a primary. Worst case, this could also take serverSelectionTimeoutMS to ultimately select a primary (e.g. wait through an election) or hit a server selection timeout.

That scenario seems quite rare, so I'm inclined to keep the code as-is rather and punt on any optimization until we actually have evidence that this causes problems down the line. IMO, the aggregate() helper is already very convoluted so anything we can do to avoid introducing more complexity is welcomed.

Verified

This commit was signed with the committer’s verified signature.
nlf nlf
Synced CRUD spec tests with mongodb/specifications@5f8f668
@jmikola jmikola merged commit 7ed839a into mongodb:master Oct 18, 2021
@jmikola jmikola deleted the phplib-651 branch October 18, 2021 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants