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

Remove MongoDBProxy and Migrate to Native PyMongo Retry Functionality #35213

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions requirements/edx/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -767,8 +767,6 @@ openedx-learning==0.10.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/kernel.in
openedx-mongodbproxy==0.2.1
# via -r requirements/edx/kernel.in
optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
Expand Down Expand Up @@ -877,7 +875,6 @@ pymongo==4.4.0
# edx-opaque-keys
# event-tracking
# mongoengine
# openedx-mongodbproxy
pynacl==1.5.0
# via
# edx-django-utils
Expand Down
5 changes: 0 additions & 5 deletions requirements/edx/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1308,10 +1308,6 @@ openedx-learning==0.10.0
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
openedx-mongodbproxy==0.2.1
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
Expand Down Expand Up @@ -1545,7 +1541,6 @@ pymongo==4.4.0
# edx-opaque-keys
# event-tracking
# mongoengine
# openedx-mongodbproxy
pynacl==1.5.0
# via
# -r requirements/edx/doc.txt
Expand Down
3 changes: 0 additions & 3 deletions requirements/edx/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -895,8 +895,6 @@ openedx-learning==0.10.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
openedx-mongodbproxy==0.2.1
# via -r requirements/edx/base.txt
optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
Expand Down Expand Up @@ -1037,7 +1035,6 @@ pymongo==4.4.0
# edx-opaque-keys
# event-tracking
# mongoengine
# openedx-mongodbproxy
pynacl==1.5.0
# via
# -r requirements/edx/base.txt
Expand Down
1 change: 0 additions & 1 deletion requirements/edx/kernel.in
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ openedx-django-require
openedx-events # Open edX Events from Hooks Extension Framework (OEP-50)
openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50)
openedx-learning # Open edX Learning core (experimental)
openedx-mongodbproxy
openedx-django-wiki
path
piexif # Exif image metadata manipulation, used in the profile_images app
Expand Down
3 changes: 0 additions & 3 deletions requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -974,8 +974,6 @@ openedx-learning==0.10.0
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
openedx-mongodbproxy==0.2.1
# via -r requirements/edx/base.txt
optimizely-sdk==4.1.1
# via
# -c requirements/edx/../constraints.txt
Expand Down Expand Up @@ -1151,7 +1149,6 @@ pymongo==4.4.0
# edx-opaque-keys
# event-tracking
# mongoengine
# openedx-mongodbproxy
pynacl==1.5.0
# via
# -r requirements/edx/base.txt
Expand Down
9 changes: 3 additions & 6 deletions xmodule/contentstore/mongo.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from bson.son import SON
from fs.osfs import OSFS
from gridfs.errors import NoFile, FileExists
from mongodb_proxy import autoretry_read

Copy link
Member

Choose a reason for hiding this comment

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

to satisfy pycodestyle, you'll need to remove these now-empty lines

from opaque_keys.edx.keys import AssetKey

from xmodule.contentstore.content import XASSET_LOCATION_TAG
Expand All @@ -39,8 +39,7 @@ def __init__(
:param collection: ignores but provided for consistency w/ other doc_store_config patterns
"""
# GridFS will throw an exception if the Database is wrapped in a MongoProxy. So don't wrap it.
# The appropriate methods below are marked as autoretry_read - those methods will handle
# the AutoReconnect errors.

self.connection_params = {
'db': db,
'host': host,
Expand Down Expand Up @@ -164,7 +163,6 @@ def delete(self, location_or_id):
# Deletes of non-existent files are considered successful
self.fs.delete(location_or_id)

@autoretry_read()
def find(self, location, throw_on_not_found=True, as_stream=False): # lint-amnesty, pylint: disable=arguments-differ
content_id, __ = self.asset_db_key(location)

Expand Down Expand Up @@ -292,7 +290,7 @@ def remove_redundant_content_for_courses(self):
self.fs_files.remove(query)
return assets_to_delete

@autoretry_read()

def _get_all_content_for_course(self,
course_key,
get_thumbnails=False,
Expand Down Expand Up @@ -424,7 +422,6 @@ def set_attrs(self, location, attr_dict):
if result.matched_count == 0:
raise NotFoundError(asset_db_key)

@autoretry_read()
def get_attrs(self, location):
"""
Gets all of the attributes associated with the given asset. Note, returns even built in attrs
Expand Down
7 changes: 0 additions & 7 deletions xmodule/modulestore/mongo/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import pymongo
from bson.son import SON
from fs.osfs import OSFS
from mongodb_proxy import autoretry_read
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator
from path import Path as path
Expand Down Expand Up @@ -578,7 +577,6 @@ def _drop_database(self, database=True, collections=True, connections=True):
if connections:
connection.close()

@autoretry_read()
def fill_in_run(self, course_key):
"""
In mongo some course_keys are used without runs. This helper function returns
Expand Down Expand Up @@ -737,7 +735,6 @@ def _load_items(self, course_key, items, using_descriptor_system=None, for_paren
for item in items
]

@autoretry_read()
def get_course_summaries(self, **kwargs):
"""
Returns a list of `CourseSummary`. This accepts an optional parameter of 'org' which
Expand Down Expand Up @@ -786,7 +783,6 @@ def extract_course_summary(course):

return courses_summaries

@autoretry_read()
def get_courses(self, **kwargs):
'''
Returns a list of course descriptors. This accepts an optional parameter of 'org' which
Expand Down Expand Up @@ -821,7 +817,6 @@ def get_courses(self, **kwargs):
)
return [course for course in base_list if not isinstance(course, ErrorBlock)]

@autoretry_read()
def _find_one(self, location):
'''Look for a given location in the collection. If the item is not present, raise
ItemNotFoundError.
Expand Down Expand Up @@ -867,7 +862,6 @@ def get_course(self, course_key, **kwargs): # lint-amnesty, pylint: disable=arg
except ItemNotFoundError:
return None

@autoretry_read()
def has_course(self, course_key, ignore_case=False, **kwargs): # lint-amnesty, pylint: disable=arguments-differ
"""
Returns the course_id of the course if it was found, else None
Expand Down Expand Up @@ -962,7 +956,6 @@ def _id_dict_to_son(id_dict):
for key in ('tag', 'org', 'course', 'category', 'name', 'revision')
])

@autoretry_read()
def get_items( # lint-amnesty, pylint: disable=arguments-differ
self,
course_id,
Expand Down
3 changes: 0 additions & 3 deletions xmodule/modulestore/split_mongo/mongo_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from django.db.transaction import TransactionManagementError
import pymongo
import pytz
from mongodb_proxy import autoretry_read
# Import this just to export it
from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import
from edx_django_utils import monitoring
Expand Down Expand Up @@ -363,7 +362,6 @@ def get_structure(self, key, course_context=None):

return structure

@autoretry_read()
def find_structures_by_id(self, ids, course_context=None):
"""
Return all structures that specified in ``ids``.
Expand All @@ -380,7 +378,6 @@ def find_structures_by_id(self, ids, course_context=None):
tagger.measure("structures", len(docs))
return docs

@autoretry_read()
def find_courselike_blocks_by_id(self, ids, block_type, course_context=None):
"""
Find all structures that specified in `ids`. Among the blocks only return block whose type is `block_type`.
Expand Down
5 changes: 0 additions & 5 deletions xmodule/modulestore/split_mongo/split.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@

from bson.objectid import ObjectId
from ccx_keys.locator import CCXBlockUsageLocator, CCXLocator
from mongodb_proxy import autoretry_read
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import (
BlockUsageLocator,
Expand Down Expand Up @@ -953,7 +952,6 @@ def _create_library_locator(self, library_info, branch):
branch=branch,
)

@autoretry_read()
def get_courses(self, branch, **kwargs): # lint-amnesty, pylint: disable=arguments-differ
"""
Returns a list of course blocks matching any given qualifiers.
Expand All @@ -969,7 +967,6 @@ def get_courses(self, branch, **kwargs): # lint-amnesty, pylint: disable=argume
# get the blocks for each course index (s/b the root)
return self._get_structures_for_branch_and_locator(branch, self._create_course_locator, **kwargs)

@autoretry_read()
def get_course_summaries(self, branch, **kwargs):
"""
Returns a list of `CourseSummary` which matching any given qualifiers.
Expand Down Expand Up @@ -1026,7 +1023,6 @@ def get_library_keys(self):
in self.find_matching_course_indexes(branch="library")
})

@autoretry_read()
def get_library_summaries(self, **kwargs):
"""
Returns a list of `LibrarySummary` objects.
Expand Down Expand Up @@ -3255,7 +3251,6 @@ def _update_block_in_structure(self, structure, block_key, content):
"""
structure['blocks'][block_key] = content

@autoretry_read()
def find_courses_by_search_target(self, field_name, field_value):
"""
Find all the courses which cached that they have the given field with the given value.
Expand Down
22 changes: 14 additions & 8 deletions xmodule/modulestore/tests/test_split_mongo_mongo_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ class TestHeartbeatFailureException(unittest.TestCase):

@patch('pymongo.MongoClient')
@patch('pymongo.database.Database')
def test_heartbeat_raises_exception_when_connection_alive_is_false(self, *calls):
# pylint: disable=W0613
with patch('mongodb_proxy.MongoProxy') as mock_proxy:
mock_proxy.return_value.admin.command.side_effect = ConnectionFailure('Test')
useless_conn = MongoPersistenceBackend('useless', 'useless', 'useless')

with pytest.raises(HeartbeatFailure):
useless_conn.heartbeat()
def test_heartbeat_retries_on_failure(self, MockDatabase, MockClient):
# Setup mock client and database
mock_client = MockClient.return_value
mock_database = MockDatabase.return_value
mock_database.admin.command.side_effect = ConnectionFailure('Test')

useless_conn = MongoPersistenceBackend('useless', 'useless', 'useless')

# Verify that the heartbeat method raises a HeartbeatFailure
with pytest.raises(HeartbeatFailure):
useless_conn.heartbeat()

# Assert that retries are handled correctly
self.assertGreater(mock_database.admin.command.call_count, 1) # Ensure retries happened
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks for testing this.

9 changes: 0 additions & 9 deletions xmodule/mongo_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import logging

import pymongo
from mongodb_proxy import MongoProxy
from pymongo.read_preferences import ( # lint-amnesty, pylint: disable=unused-import
ReadPreference,
_MONGOS_MODES,
Expand Down Expand Up @@ -66,14 +65,6 @@ def connect_to_mongodb(
connection_params.update({'username': user, 'password': password, 'authSource': db})

mongo_conn = pymongo.MongoClient(**connection_params)

if proxy:
Copy link
Member

Choose a reason for hiding this comment

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

Since proxy is now unused, could you remove it from the parameters list and the docstring for this method?

mongo_conn = MongoProxy(
mongo_conn[db],
wait_time=retry_wait_time
)
return mongo_conn

return mongo_conn[db]


Expand Down
Loading