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

CDRIVER-4099 Support mongos redirection during retryable operations #1529

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

eramongodb
Copy link
Collaborator

@eramongodb eramongodb commented Feb 6, 2024

Resolves CDRIVER-4099. Verified by this patch (filtered to relevant tasks; CSE tasks are failing atm due to an unrelated issue).

This PR also implements the proposed prose test spec changes in mongodb/specifications#1504.

Description

There are five internal functions in libmongoc that each individually (re)implement retryable reads/writes:

  • _mongoc_client_retryable_write_command_with_stream
  • _mongoc_client_retryable_read_command_with_stream
  • mongoc_collection_find_and_modify_with_opts
  • _mongoc_cursor_run_command
  • _mongoc_write_opmsg

This PR refrains from refactoring these instances to use a comment implementation due to the complexity required. Instead, the new mongoc_deprioritized_servers_add_if_sharded function serves to document the instances of retry-op code paths for future reference and make a first step towards unifying the behavior of these disparate implementations in the future.

The mongoc_deprioritized_servers_add_if_sharded is a convenient function that only adds the provided server to the set of deprioritized servers when the topology is Sharded, per spec:

In a sharded cluster, the server on which the operation failed MUST be provided to the server selection mechanism as a deprioritized server.

The mongoc_deprioritized_servers_t interface deliberately operates on mongoc_server_description_t objects, rather than the underlying server IDs currently used as keys in the set, to leave the path open for changing the method used to identify servers (i.e. to host_and_port) or using an array instead of a set (can't use strings as keys in mongoc_set_t) if needed without requiring a significant refactor.

The bulk of changes in this PR are to accomodate the addition of a new parameter to all intermediate internal functions that must forward the deprioritized servers list aaaaall the way down to mongoc_topology_description_suitable_servers where it is ultimately used, per spec:

Find suitable servers by topology type and operation type. If a list of deprioritized servers is provided, and the topology is a sharded cluster, these servers should be selected only if there are no other suitable servers. The server selection algorithm MUST ignore the deprioritized servers if the topology is not a sharded cluster.

This was interpreted as being most appropriately implemented in mongoc_topology_description_suitable_servers in the "Sharded clusters" block immediately before filtering for suitable mongoses.

Trace log messages are used to verify that deprioritization code paths are being executed as intended by the prose tests. All relevant trace logs use the deprioritization: prefix for easy filtering in the verbose trace logs, e.g. ./test-libmongoc -t | grep "deprioritization" (simplified):

mongoc_deprioritized_servers_add_if_sharded():2710 deprioritization: add to list: localhost:27018 (id: 2)
_mongoc_filter_deprioritized_servers():562 deprioritization: filtering list of candidates
_mongoc_filter_deprioritized_servers():574 deprioritization: - kept: localhost:27017 (id: 1)
_mongoc_filter_deprioritized_servers():579 deprioritization: - removed: localhost:27018 (id: 2)
_mongoc_filter_deprioritized_servers():591 deprioritization: using filtered list of candidates
mongoc_deprioritized_servers_add_if_sharded():2710 deprioritization: add to list: localhost:27017 (id: 1)
_mongoc_filter_deprioritized_servers():562 deprioritization: filtering list of candidates
_mongoc_filter_deprioritized_servers():579 deprioritization: - removed: localhost:27017 (id: 1)
_mongoc_filter_deprioritized_servers():585 deprioritization: reverted due to no other suitable servers

The on_other_mongos prose tests for both retryable read and retryable write are repeated several times in an effort to reduce the possibility of (incorrectly using) normal SDAM behavior happening to produce the same result as server deprioritization due to randomized server selection. Otherwise the test may occasionally succeed (false-positive) even without the implementation of server deprioritization. The loops can be removed if considered excessive/slow.

A drive-by fix moves the MONGOC_INFO message for mock server startup to a position where the printed port number is accurate to the actual port of the running server.

Copy link
Collaborator

@kevinAlbs kevinAlbs 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 overall. Suggested one behavior change and minor test suggestions.

src/libmongoc/tests/test-mongoc-retryable-reads.c Outdated Show resolved Hide resolved
src/libmongoc/tests/test-mongoc-retryable-reads.c Outdated Show resolved Hide resolved
src/libmongoc/tests/test-mongoc-retryable-writes.c Outdated Show resolved Hide resolved
src/libmongoc/tests/test-mongoc-retryable-writes.c Outdated Show resolved Hide resolved
src/libmongoc/tests/test-mongoc-retryable-writes.c Outdated Show resolved Hide resolved
src/libmongoc/tests/test-mongoc-retryable-writes.c Outdated Show resolved Hide resolved
src/libmongoc/src/mongoc/mongoc-topology-private.h Outdated Show resolved Hide resolved
@eramongodb eramongodb merged commit acc9ea8 into mongodb:master Feb 13, 2024
30 of 33 checks passed
@eramongodb eramongodb deleted the cdriver-4099 branch February 13, 2024 18:43
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.

2 participants