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

PYTHON-2998 Remove md5 checksums from gridfs and remove disable_md5 #776

Merged
merged 5 commits into from
Nov 5, 2021
Merged

PYTHON-2998 Remove md5 checksums from gridfs and remove disable_md5 #776

merged 5 commits into from
Nov 5, 2021

Conversation

ShaneHarvey
Copy link
Member

@ShaneHarvey ShaneHarvey commented Nov 4, 2021 •
edited
Loading

No description provided.

@ShaneHarvey ShaneHarvey marked this pull request as ready for review November 4, 2021 20:32
@ShaneHarvey ShaneHarvey requested a review from juliusgeo November 4, 2021 20:32
behackett
behackett requested changes Nov 4, 2021
View reviewed changes
doc/changelog.rst Outdated
- :class:`~pymongo.mongo_client.MongoClient`` now raises an
:exc:`~pymongo.errors.InvalidURI` exception
when it encounters unescaped percent signs in username and password when
parsing MongoDB URIs.
- Removed the `disable_md5` parameter for :class:`~gridfs.GridFSBucket` and
:class:`~gridfs.GridFS`. MD5 checksums are now always disabled in GridFS.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of saying "checksums are now always disabled" I think you should say something like "gridfs no longer generates a checksum" and link to the guidance in the upgrade docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

doc/migrate-to-pymongo4.rst Outdated
................................

Removed the `disable_md5` option for :class:`~gridfs.GridFSBucket` and
:class:`~gridfs.GridFS`. MD5 checksums are now always disabled in GridFS.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here about "are now always disabled".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

gridfs/__init__.py Outdated
cannot be used for regulatory or other reasons. Defaults to False.

.. versionchanged:: 4.0
Removed the `disable_md5` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Also mention that gridfs no longer generates an md5sum

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to link to the migration guide which describes the change in more detail.

gridfs/__init__.py Outdated
cannot be used for regulatory or other reasons. Defaults to False.

.. versionchanged:: 4.0
Removed the `disable_md5` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to link to the migration guide which describes the change in more detail.

gridfs/grid_file.py Outdated
- `**kwargs` (optional): file level options (see above)

.. versionchanged:: 4.0
Removed the `disable_md5` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

And here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to link to the migration guide which describes the change in more detail.

@ShaneHarvey ShaneHarvey requested a review from behackett November 4, 2021 22:34
juliusgeo
juliusgeo approved these changes Nov 4, 2021
View reviewed changes
Copy link
Contributor

@juliusgeo juliusgeo left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShaneHarvey
Copy link
Member Author

ShaneHarvey commented Nov 4, 2021 •
edited
Loading

I made a small test update to use delete_many+drop_indexes to clear the collection state rather than use drop_collection. This speeds up the gridfs test quite a bit. On a MongoDB 5.1.0-alpha-1170-g4c2c9d1 3 member replica set the following used to run in 219 seconds:

$ python3 -m unittest discover -s test -v -k 'grid'  
...
----------------------------------------------------------------------
Ran 133 tests in 219.580s

OK

The same command now runs in 102.092s. So we've shaved off about 2 minutes in runtime! Still 100 seconds seems high for these tests. There's probably something else we could do but I'll save that for a future project.

@behackett
Copy link
Member

Still LGTM

@ShaneHarvey ShaneHarvey merged commit e271315 into mongodb:master Nov 5, 2021
juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 5, 2022
@ShaneHarvey @juliusgeo
PYTHON-2998 Remove md5 checksums from gridfs and remove disable_md5 (m…
4060c75 
…ongodb#776)

Speed up gridfs tests (shaves off about 2 minutes on macOS).
juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 7, 2022
@ShaneHarvey @juliusgeo
PYTHON-2998 Remove md5 checksums from gridfs and remove disable_md5 (m…
30062e3 
…ongodb#776)

Speed up gridfs tests (shaves off about 2 minutes on macOS).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behackett behackett behackett approved these changes

@juliusgeo juliusgeo juliusgeo 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
@ShaneHarvey @behackett @juliusgeo