PYTHON-1934 retryWrites=True with MMAPv1 storage engine should raise an actionable error

Author: prashantmital

.evergreen/config.yml

@@ -1471,7 +1471,9 @@ buildvariants:
       then:
         add_tasks:
           - "test-4.0-standalone"
+          - "test-4.0-replica_set"
           - "test-3.6-standalone"
+          - "test-3.6-replica_set"
           - "test-3.4-standalone"
           - "test-3.2-standalone"
     - if:

doc/changelog.rst

@@ -68,6 +68,17 @@ Version 3.9 adds support for MongoDB 4.2. Highlights include:
 - Connections now survive primary step-down. Applications should expect less
   socket connection turnover during replica set elections.
 
+Unavoidable breaking changes:
+
+- Applications that use MongoDB with the MMAPv1 storage engine must now
+  explicitly disable retryable writes via the connection string
+  (e.g. ``MongoClient("mongodb://my.mongodb.cluster/db?retryWrites=false")``) or
+  the :class:`~pymongo.mongo_client.MongoClient` constructor's keyword argument
+  (e.g. ``MongoClient("mongodb://my.mongodb.cluster/db", retryWrites=False)``)
+  to avoid running into :class:`~pymongo.errors.OperationFailure` exceptions
+  during write operations. The MMAPv1 storage engine is deprecated and does
+  not support retryable writes which are now turned on by default.
+
 .. _URI options specification: https://github.com/mongodb/specifications/blob/master/source/uri-options/uri-options.rst
 
 

pymongo/mongo_client.py

@@ -1397,6 +1397,14 @@ def is_retrying():
                     retrying = True
                 last_error = exc
             except OperationFailure as exc:
+                # retryWrites on MMAPv1 should raise an actionable error.
+                if (exc.code == 20 and
+                        str(exc).startswith("Transaction numbers")):
+                    errmsg = (
+                        "This MongoDB deployment does not support "
+                        "retryable writes. Please add retryWrites=false "
+                        "to your connection string.")
+                    raise OperationFailure(errmsg, exc.code, exc.details)
                 if not retryable or is_retrying():
                     raise
                 if exc.code not in helpers._RETRYABLE_ERROR_CODES:

test/__init__.py

@@ -256,6 +256,10 @@ def _init_client(self):
                 self.cmd_line = self.client.admin.command('getCmdLineOpts')
 
             self.server_status = self.client.admin.command('serverStatus')
+            if self.storage_engine == "mmapv1":
+                # MMAPv1 does not support retryWrites=True.
+                self.default_client_options['retryWrites'] = False
+
             ismaster = self.ismaster
             self.sessions_enabled = 'logicalSessionTimeoutMinutes' in ismaster
 
@@ -349,6 +353,14 @@ def has_secondaries(self):
             return False
         return bool(len(self.client.secondaries))
 
+    @property
+    def storage_engine(self):
+        try:
+            return self.server_status.get("storageEngine", {}).get("name")
+        except AttributeError:
+            # Raised if self.server_status is None.
+            return None
+
     def _check_user_provided(self):
         """Return True if db_user/db_password is already an admin user."""
         client = pymongo.MongoClient(
@@ -449,14 +461,7 @@ def require_no_mmap(self, func):
         def is_not_mmap():
             if self.is_mongos:
                 return True
-            try:
-                storage_engine = self.server_status.get(
-                    'storageEngine').get('name')
-            except AttributeError:
-                # Raised if the storageEngine key does not exist or if
-                # self.server_status is None.
-                return False
-            return storage_engine != 'mmapv1'
+            return self.storage_engine != 'mmapv1'
 
         return self._require(
             is_not_mmap, "Storage engine must not be MMAPv1", func=func)
@@ -618,11 +623,15 @@ def require_sessions(self, func):
                              func=func)
 
     def supports_transactions(self):
+        if self.storage_engine == 'mmapv1':
+            return False
+
         if self.version.at_least(4, 1, 8):
             return self.is_mongos or self.is_rs
 
         if self.version.at_least(4, 0):
             return self.is_rs
+
         return False
 
     def require_transactions(self, func):

test/test_examples.py

@@ -663,6 +663,7 @@ def test_delete(self):
 
     @client_context.require_version_min(3, 5, 11)
     @client_context.require_replica_set
+    @client_context.require_no_mmap
     def test_change_streams(self):
         db = client_context.client.pymongo_test
         done = False
@@ -1117,6 +1118,7 @@ def shipment_transaction(session):
 class TestCausalConsistencyExamples(IntegrationTest):
     @client_context.require_version_min(3, 6, 0)
     @client_context.require_secondaries_count(1)
+    @client_context.require_no_mmap
     def test_causal_consistency(self):
         # Causal consistency examples
         client = self.client

test/test_retryable_reads.py

@@ -54,6 +54,8 @@ class TestSpec(SpecRunner):
 
     @classmethod
     @client_context.require_version_min(4, 0)
+    # TODO: remove this once PYTHON-1948 is done.
+    @client_context.require_no_mmap
     def setUpClass(cls):
         super(TestSpec, cls).setUpClass()
         if client_context.is_mongos and client_context.version[:2] <= (4, 0):
@@ -65,8 +67,13 @@ def maybe_skip_scenario(self, test):
             'listCollectionObjects', 'listIndexNames', 'listDatabaseObjects']
         for name in skip_names:
             if name.lower() in test['description'].lower():
-                raise unittest.SkipTest(
-                    'PyMongo does not support %s' % (name,))
+                self.skipTest('PyMongo does not support %s' % (name,))
+
+        # Skip changeStream related tests on MMAPv1.
+        test_name = self.id().rsplit('.')[-1]
+        if ('changestream' in test_name.lower() and
+                client_context.storage_engine == 'mmapv1'):
+            self.skipTest("MMAPv1 does not support change streams.")
 
     def get_scenario_coll_name(self, scenario_def):
         """Override a test's collection name to support GridFS tests."""

test/test_retryable_writes.py

@@ -80,6 +80,7 @@ def run_test_ops(self, sessions, collection, test):
 
 def create_test(scenario_def, test, name):
     @client_context.require_test_commands
+    @client_context.require_no_mmap
     def run_scenario(self):
         self.run_scenario(scenario_def, test)
 
@@ -176,9 +177,41 @@ def tearDownClass(cls):
         super(IgnoreDeprecationsTest, cls).tearDownClass()
 
 
+class TestRetryableWritesMMAPv1(IgnoreDeprecationsTest):
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestRetryableWritesMMAPv1, cls).setUpClass()
+        # Speed up the tests by decreasing the heartbeat frequency.
+        cls.knobs = client_knobs(heartbeat_frequency=0.1,
+                                 min_heartbeat_interval=0.1)
+        cls.knobs.enable()
+        cls.client = rs_or_single_client(retryWrites=True)
+        cls.db = cls.client.pymongo_test
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.knobs.disable()
+
+    @client_context.require_version_min(3, 5)
+    @client_context.require_no_standalone
+    def test_actionable_error_message(self):
+        if client_context.storage_engine != 'mmapv1':
+            raise SkipTest('This cluster is not running MMAPv1')
+
+        expected_msg = ("This MongoDB deployment does not support retryable "
+                        "writes. Please add retryWrites=false to your "
+                        "connection string.")
+        for method, args, kwargs in retryable_single_statement_ops(
+                self.db.retryable_write_test):
+            with self.assertRaisesRegex(OperationFailure, expected_msg):
+                method(*args, **kwargs)
+
+
 class TestRetryableWrites(IgnoreDeprecationsTest):
 
     @classmethod
+    @client_context.require_no_mmap
     def setUpClass(cls):
         super(TestRetryableWrites, cls).setUpClass()
         # Speed up the tests by decreasing the heartbeat frequency.
@@ -429,6 +462,7 @@ def test_batch_splitting_retry_fails(self):
 class TestRetryableWritesTxnNumber(IgnoreDeprecationsTest):
     @client_context.require_version_min(3, 6)
     @client_context.require_replica_set
+    @client_context.require_no_mmap
     def test_increment_transaction_id_without_sending_command(self):
         """Test that the txnNumber field is properly incremented, even when
         the first attempt fails before sending the command.

test/test_session.py

@@ -1057,6 +1057,7 @@ def test_server_not_causal(self):
             self.assertIsNone(act)
 
     @client_context.require_no_standalone
+    @client_context.require_no_mmap
     def test_read_concern(self):
         with self.client.start_session(causal_consistency=True) as s:
             coll = self.client.pymongo_test.test

test/utils_spec_runner.py

@@ -467,6 +467,10 @@ def run_scenario(self, scenario_def, test):
         listener = OvertCommandListener()
         # Create a new client, to avoid interference from pooled sessions.
         client_options = self.parse_client_options(test['clientOptions'])
+        # MMAPv1 does not support retryable writes.
+        if (client_options.get('retryWrites') is True and
+                client_context.storage_engine == 'mmapv1'):
+            self.skipTest("MMAPv1 does not support retryWrites=True")
         use_multi_mongos = test['useMultipleMongoses']
         if client_context.is_mongos and use_multi_mongos:
             client = rs_client(client_context.mongos_seeds(),