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.

Sign up for GitHub

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

Jump to bottom

Upgrade pymongo #34675

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Upgrade pymongo #34675

merged 1 commit into from
Jul 25, 2024

Conversation

mumarkhan999
Copy link
Member

@mumarkhan999 mumarkhan999 commented May 2, 2024 •
edited
Loading

Issue Link

Description

Related PRs

@mumarkhan999 mumarkhan999 requested a review from a team as a code owner May 2, 2024 11:45
@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 11 times, most recently from b0683c5 to 4139dea Compare May 3, 2024 15:46
@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 4 times, most recently from 424c152 to 42fd48e Compare May 22, 2024 07:10
@mumarkhan999 mumarkhan999 reopened this May 22, 2024
@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 11 times, most recently from 39a0ef6 to dd4fca6 Compare May 22, 2024 13:14
@bmtcril bmtcril mentioned this pull request Jul 15, 2024
Merged
6 tasks
feanil
feanil reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

A couple of questions but this generally makes sense to me.

conf/locale/en/LC_MESSAGES/.gitignore Outdated
@@ -1,3 +0,0 @@
# We want to ignore files in this directory which we do in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this message? It seems useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is temporary to make sandbox builds successful.
Will be reverted before this pr is merged.

xmodule/contentstore/mongo.py Outdated
@@ -142,12 +181,17 @@ def find(self, location, throw_on_not_found=True, as_stream=False): # lint-amne
'thumbnail',
thumbnail_location[4]
)

# md5 = getattr(fp, 'md5', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these comments still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed. Removed.

@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 2 times, most recently from 1a06f6a to b840069 Compare July 18, 2024 13:03
feanil
feanil reviewed Jul 18, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

One small question, but the approach generally seems good, There are a lot of new very small functions check_connection do_connection, etc. They made it harder for me to read through what was happening, was there a reason for making these new functions?

requirements/edx/base.txt Outdated
@@ -81,7 +81,7 @@ bridgekeeper==0.9
# via -r requirements/edx/kernel.in
camel-converter[pydantic]==3.1.2
# via meilisearch
celery==5.4.0
celery==5.3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is celery being downgraded?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mumarkhan999 still waiting for an answer to why this happened.

@mumarkhan999
Copy link
Member Author

mumarkhan999 commented Jul 19, 2024 •
edited
Loading

One small question, but the approach generally seems good, There are a lot of new very small functions check_connection do_connection, etc. They made it harder for me to read through what was happening, was there a reason for making these new functions?

These functions are needed to manage the availability of MongoDB connections.

In pymongo versions >4.x.xx the reconnection functionality has been removed.
(More info is available in the changelog) Now, these functions are making sure that our connection is open while making any MongoDB request.

@mumarkhan999 mumarkhan999 requested a review from feanil July 19, 2024 14:09
feanil
feanil reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Generally looks good to me and makes sense. I just have one last question about the downgrade of the celery dependency.

requirements/edx/base.txt Outdated
@@ -81,7 +81,7 @@ bridgekeeper==0.9
# via -r requirements/edx/kernel.in
camel-converter[pydantic]==3.1.2
# via meilisearch
celery==5.4.0
celery==5.3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

@mumarkhan999 still waiting for an answer to why this happened.

@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 5 times, most recently from 64af8db to b176a43 Compare July 23, 2024 13:10
@mumarkhan999 mumarkhan999 requested a review from feanil July 23, 2024 13:34
@mumarkhan999 mumarkhan999 force-pushed the umar/upgrade-pymongo branch 2 times, most recently from 92c31ac to 5fbc2e7 Compare July 24, 2024 10:23
feanil
feanil approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Some small nits and suggestion but I think once those are addressed this can be merged. I don't need to do another round of review unless there are significant changes.

conf/locale/en/LC_MESSAGES/.gitignore Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to revert this change before merging.

requirements/constraints.txt Outdated
@@ -34,7 +34,10 @@ django-simple-history==3.4.0

# constrained in opaque_keys. migration guide here: https://pymongo.readthedocs.io/en/4.0/migrate-to-pymongo4.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment need to be updated now?

requirements/constraints.txt
pymongo<4.0.0
pymongo<4.4.1

# To override the constraint of edx-lint
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# To override the constraint of edx-lint
# To override the constraint of edx-lint
# This can be removed once https://github.com/openedx/edx-platform/issues/34586 is resolved
# and the upstream constraint in edx-lint has been removed.

@mumarkhan999 mumarkhan999 merged commit cbd4904 into master Jul 25, 2024
49 checks passed
@mumarkhan999 mumarkhan999 deleted the umar/upgrade-pymongo branch July 25, 2024 11:03
mumarkhan999 added a commit that referenced this pull request Jul 25, 2024
@mumarkhan999
Revert "feat!: upgrade pymongo (#34675)"
e75ad02 
This reverts commit cbd4904.
@mumarkhan999 mumarkhan999 mentioned this pull request Jul 25, 2024
Merged
mumarkhan999 added a commit that referenced this pull request Jul 25, 2024
@mumarkhan999
Revert "feat!: upgrade pymongo (#34675)" (#35178)
5198496 
This reverts commit cbd4904.
@mumarkhan999 mumarkhan999 restored the umar/upgrade-pymongo branch July 25, 2024 13:46
@mumarkhan999 mumarkhan999 mentioned this pull request Jul 25, 2024
Merged
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@adamstankiewicz adamstankiewicz mentioned this pull request Aug 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mumarkhan999 @edx-pipeline-bot @feanil