CXX-1998 'CommitQuorum' option support for 'createIndexes’ command on MongoDB 4.4 #691

bazile-clyde · 2020-07-10T21:00:08Z

No description provided.

bazile-clyde · 2020-07-10T21:05:55Z

@kevinAlbs I see you closed CDRIVER-3627. From the spec:

Q: Why does the driver manually throw errors if the commitQuorum option is specified against a pre 4.4 server?
Starting in 3.4, the server validates all options passed to the createIndexes command, but due to a bug in versions 4.2.0-4.2.5 of the server (SERVER-47193), specifying commitQuorum does not result in an error. The option is used interally by the server on those versions, and its value could have adverse effects on index builds. To prevent users from mistakenly specifying this option, drivers manually verify it is only sent to 4.4+ servers.

I'm not sure if this was missed in C or it was decided that we shouldn't do it. This implementation doesn't currently throw an error for server version less than 4.4. If we decided against it in C then I'll add it here but if it was left out we should probably throw an error from libmongoc.

codecov-commenter · 2020-07-10T21:34:04Z

Codecov Report

Merging #691 into master will decrease coverage by 2.17%.
The diff coverage is 24.07%.

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
- Coverage   85.79%   83.61%   -2.18%     
==========================================
  Files         346      346              
  Lines       15521    16009     +488     
==========================================
+ Hits        13316    13386      +70     
- Misses       2205     2623     +418

Impacted Files	Coverage Δ
src/mongocxx/index_view.hpp	`100.00% <ø> (ø)`
src/mongocxx/options/index_view.hpp	`100.00% <ø> (ø)`
src/mongocxx/test/index_view.cpp	`91.94% <16.66%> (-7.26%)`	⬇️
src/mongocxx/private/index_view.hh	`84.31% <18.75%> (-12.24%)`	⬇️
src/mongocxx/options/index_view.cpp	`47.61% <20.00%> (-25.11%)`	⬇️
src/mongocxx/collection.cpp	`90.73% <100.00%> (ø)`
src/mongocxx/index_view.cpp	`95.23% <100.00%> (ø)`
src/mongocxx/private/libmongoc_symbols.hh	`100.00% <100.00%> (ø)`
src/mongocxx/test/client_side_encryption.cpp	`9.17% <0.00%> (-0.69%)`	⬇️
src/mongocxx/test_util/client_helpers.cpp	`22.96% <0.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 510d2b0...64f220a. Read the comment docs.

kevinAlbs · 2020-07-13T21:13:21Z

I'm not sure if this was missed in C or it was decided that we shouldn't do it. This implementation doesn't currently throw an error for server version less than 4.4. If we decided against it in C then I'll add it here but if it was left out we should probably throw an error from libmongoc.

It was not applicable for libmongoc. The only index creation helpers in libmongoc are deprecated (and not used by the C++ driver), and the generic command helpers should not be inspecting the command options.

To do the wire version check in the C++ driver, the only way I see is to use libmongoc's mongoc_client_select_server and inspect the wire version of the primary's server description using mongoc_server_description_ismaster. Neither function is used by the C++ driver, so you'll need to add it to libmongoc_symbols.hh. But... there's more. mongoc_client_select_server requires a mongoc_client_t*, but mongocxx::index_view only has a mongoc_collection_t*. I think mongocxx::collection::indexes() will need to additionally pass that through, which it can get from _get_impl()->client_impl->client_t.

TBH it seems like a very roundabout way for this check, but it also might be the best option we have right now. This is an unfortunate consequence of libmongoc not providing index helpers, it pushes more work onto wrapping drivers :/ I think this would be an argument for adding index helpers to libmongoc. I do not know the rationale for why they were deprecated originally.

Edit: Thinking about this a bit more, even if it is an exception, checking the wire version constraint may be worth doing in libmongoc. I don't think it'd be harmful to add the check, and it would make it a lot easier for wrapping drivers. I'll try taking a crack at it in CDRIVER-3748.

src/mongocxx/options/index_view.hpp

src/mongocxx/test/index_view.cpp

src/mongocxx/private/index_view.hh

… MongoDB 4.4'

kevinAlbs

Looking good, just some comments about handling NULL / freeing the server description.

src/mongocxx/private/index_view.hh

kevinAlbs · 2020-07-15T18:17:47Z

src/mongocxx/private/index_view.hh

+            auto is_master = libmongoc::server_description_ismaster(server_description);
+
+            bson_iter_t iter;
+            bson_iter_init_find(&iter, is_master, "maxWireVersion");


This could return false if the ismaster reply does not contain maxWireVersion. I don't think that would realistically happen (if there was no primary available, then mongoc_client_select_server would have returned NULL). But I think it is worth checking. If no maxWireVersion is available, throw an error.

src/mongocxx/private/index_view.hh

src/mongocxx/private/libmongoc_symbols.hh

kevinAlbs · 2020-07-15T20:44:30Z

src/mongocxx/test/index_view.cpp

+            Catch::Matches("(.*)commit( )?quorum(.*)", Catch::CaseSensitive::No);
+
+        using namespace test_util;
+        bool is_supported = compare_versions(get_server_version(mongodb_client), "4.4") >= 0 &&


The test failure on s390x seems related to the version of MongoDB running.

The logs of:
https://evergreen.mongodb.com/task/cxx_driver_ubuntu1604_zseries_compile_and_test_with_shared_libs_patch_510d2b0c04d1103bd1252088cda78522346b1f25_5f0f5f7f7742ae3af8c73ca8_20_07_15_19_56_48

say:

db version v4.3.0-2027-gbe045fe

That test is running against single, so is_supported would be false, and an exception is expected. But since that server version is likely reporting wire version 9, your wire version check is not getting hit. I think the right thing to do is change this comparison to check the wire version instead:

bool is_supported = get_max_wire_version (mongodb_client) >= 9 && get_topology (mongoc_client) != "single";

kevinAlbs · 2020-07-15T20:47:40Z

src/mongocxx/private/index_view.hh

+
+    class scoped_server_description {
+       public:
+        explicit scoped_server_description(mongoc_server_description_t* sd) : sd(sd) {}


Oh nice, I forgot about marking that explicit.

kevinAlbs

Changes LGTM. I still believe the failing zseries test is due to running against 4.3 and is not a bug in this implementation. I added a suggestion to resolve that.

kevinAlbs · 2020-07-17T14:33:15Z

src/mongocxx/test/index_view.cpp

+            REQUIRE_THROWS_WITH(indexes.create_one(model, options), commit_quorum_regex);
+        }
+
+        SECTION("works with string") {


This section, and the section below, appear to be the ones failing.

As the capturing of is_supported shows, is_supported is false, an an exception is expected.

IIUC an exception is expected because the test runs against a single topology, but "majority" should only be supported on a replica set. And the section below expects a server to always reject the value "bad_str"

I still believe this is due to the intermediate 4.3.0 server version this is running against on zSeries. The server may have implemented validation after 4.3.0, so the reported wire version is still 9, but invalid commitQuorum values are still accepted. https://jira.mongodb.org/browse/SERVER-41846 in particular (which has fixVersion 4.3.1) seems related.

We could investigate further and check with the server team. But IMO, it is less important that we test server side validation of these values, and more important that we test that commitQuroum works when supported, and that the driver returns a client side error if the wire version is < 9.

I'd argue for removing the "fails with invalid string" test below, and only testing the "is_supported" case here.

Per our discussion, I skipped single typologies and got rid of the invalid with... tests.

kevinAlbs

LGTM with a warning log.

kevinAlbs · 2020-07-17T18:11:28Z

src/mongocxx/test/index_view.cpp

    }

    SECTION("commitQuorum option") {
+        if (test_util::get_topology(mongodb_client) == "single")


Can you add a warning for consistency with other skips? WARN("skip: commitQuorum option requires a replica set");

bazile-clyde force-pushed the CXX-1998 branch from 6405b39 to 2d39387 Compare July 10, 2020 21:01

bazile-clyde requested a review from kevinAlbs July 10, 2020 21:01

kevinAlbs requested changes Jul 13, 2020

View reviewed changes

jmikola reviewed Jul 14, 2020

View reviewed changes

src/mongocxx/private/index_view.hh Show resolved Hide resolved

jmikola mentioned this pull request Jul 14, 2020

CDRIVER-3748 add warning about commitQuorum mongodb/mongo-c-driver#662

Merged

bazile-clyde force-pushed the CXX-1998 branch from 2d39387 to 55e3aac Compare July 15, 2020 16:51

Clyde Bazile added 3 commits July 15, 2020 13:20

CXX-1998 'CommitQuorum' option support for 'createIndexes’ command on…

cfbff4d

… MongoDB 4.4'

address comments

f4780f8

throw err on unsupported wire version

43005fd

bazile-clyde force-pushed the CXX-1998 branch from 55e3aac to 43005fd Compare July 15, 2020 17:20

bazile-clyde requested review from jmikola and kevinAlbs July 15, 2020 17:22

remove unused headers

a4630ce

bazile-clyde force-pushed the CXX-1998 branch from 3bd0d84 to a4630ce Compare July 15, 2020 17:27

kevinAlbs requested changes Jul 15, 2020

View reviewed changes

kevinAlbs reviewed Jul 15, 2020

View reviewed changes

src/mongocxx/private/libmongoc_symbols.hh Outdated Show resolved Hide resolved

address comments

cd7f158

kevinAlbs reviewed Jul 15, 2020

View reviewed changes

use wire version

bb8c358

kevinAlbs approved these changes Jul 17, 2020

View reviewed changes

skip single typologies

63e5a70

bazile-clyde requested a review from kevinAlbs July 17, 2020 18:06

kevinAlbs approved these changes Jul 17, 2020

View reviewed changes

add warning

64f220a

bazile-clyde merged commit 5b73b8d into mongodb:master Jul 17, 2020

bazile-clyde deleted the CXX-1998 branch July 17, 2020 18:42

CXX-1998 'CommitQuorum' option support for 'createIndexes’ command on MongoDB 4.4 #691

CXX-1998 'CommitQuorum' option support for 'createIndexes’ command on MongoDB 4.4 #691

Uh oh!

Conversation

bazile-clyde commented Jul 10, 2020

Uh oh!

bazile-clyde commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kevinAlbs commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bazile-clyde commented Jul 10, 2020 •

edited

Loading

codecov-commenter commented Jul 10, 2020 •

edited

Loading

kevinAlbs commented Jul 13, 2020 •

edited

Loading