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

Update for PyMongo 4 and add GitHub Actions #845

Merged
merged 18 commits into from
Dec 17, 2021

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Dec 8, 2021

Description of changes

  • Adds GitHub Actions testing
  • Adds Python 3.10 support - Fixes Python 3.10 compatibility #841
  • Adds pymongo 4.0 support - Fixes mlaunch: Test with PyMongo 4.0 #844
    • Adds handling of directConnection
    • Fixed use of pymongo.database.Database.authenticate
    • Fixed use of pymongo.database.Database.add_user
    • Fixed use of pymongo.collection.Collection.insert
    • Fixed use of pymongo.collection.Collection.update
    • Fixed use of pymongo.collection.Collection.ensure_index
    • Fixed use of pymongo.collection.Collection.count
    • Fixed use of pymongo.collection.Collection.group
  • Adds pytest support (needed for GitHub Actions support) - Fixes Replace Nose with an actively maintained unit testing library #842

Testing

  • GitHub action on my fork
  • Locally running tox on macOS

O/S testing:

O/S Version(s)
Linux Ubuntu 20.04.3
macOS 10.15.7

@blink1073 blink1073 marked this pull request as draft December 8, 2021 23:09
@blink1073
Copy link
Contributor Author

blink1073 commented Dec 8, 2021

Converting to draft while I continue to work through the pymongo 4.0 changelog

Copy link
Collaborator

@stennie stennie left a comment

Choose a reason for hiding this comment

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

Epic! Thanks for the PR @blink1073 !

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
mtools/mlaunch/mlaunch.py Outdated Show resolved Hide resolved
import mtools
from mtools.mloginfo.mloginfo import MLogInfoTool
from mtools.util.logfile import LogFile


@pytest.fixture(scope="function", autouse=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice monkeypatch. Ideally we should be unit testing output of run() rather than grubbing from stdout, but we can fix that later.

test-requirements.txt Outdated Show resolved Hide resolved
@blink1073
Copy link
Contributor Author

I went through all of the deprecated functions, and the build is passing, taking this out of draft

@blink1073 blink1073 marked this pull request as ready for review December 9, 2021 00:49
mtools/util/profile_collection.py Outdated Show resolved Hide resolved
mtools/mlaunch/mlaunch.py Outdated Show resolved Hide resolved
@blink1073 blink1073 requested a review from stennie December 9, 2021 01:38
mtools/mlaunch/mlaunch.py Outdated Show resolved Hide resolved
mtools/mlaunch/mlaunch.py Show resolved Hide resolved
mtools/util/presplit.py Outdated Show resolved Hide resolved
'nChunks': { '$sum': 1 }
}
}
])
print(', '.join(["%s: %i" % (ch['shard'], ch['nChunks'])
Copy link
Contributor

@ShaneHarvey ShaneHarvey Dec 9, 2021

Choose a reason for hiding this comment

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

This fails because the 'shard' field doesn't exist:

>>> chunk_group = client['config']['chunks'].aggregate([{ '$match': { 'ns': 'test.test2' }},{ '$group': { '_id': '$shard','nChunks': { '$sum': 1 }}}])
>>> list(chunk_group)
[{'_id': 'demo-set-0', 'nChunks': 1}]
>>> chunk_group = client['config']['chunks'].aggregate([{ '$match': { 'ns': 'test.test2' }},{ '$group': { '_id': '$shard','nChunks': { '$sum': 1 }}}])
>>> print(', '.join(["%s: %i" % (ch['shard'], ch['nChunks']) for ch in chunk_group]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in <listcomp>
KeyError: 'shard'

To fix this we need to add the shard field which is a little awkward in the $group stage. AFAICT you need to use the $first accumulator:

>>> chunk_group = client['config']['chunks'].aggregate([{ '$match': { 'ns': 'test.test2' }},{ '$group': { '_id': '$shard','nChunks': { '$sum': 1 }, 'shard': {'$first': '$shard'}}}])
>>> print(', '.join(["%s: %i" % (ch['shard'], ch['nChunks']) for ch in chunk_group]))                                                                      
demo-set-0: 1

Also this code will not work at all on 5.0 because they removed the "ns" field from the chunks collection. Compare https://docs.mongodb.com/v5.0/reference/config-database/#mongodb-data-config.chunks with https://docs.mongodb.com/v4.4/reference/config-database/#mongodb-data-config.chunks . The config database is internal so can change from release to release. This particular issue should be handled in a new ticket.

Copy link
Contributor

@ShaneHarvey ShaneHarvey Dec 10, 2021

Choose a reason for hiding this comment

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

Sorry, looking at this again there's no need for the 'shard': {'$first': '$shard'}. We're already grouping on the shard field so we can change the list comprehension to use the _id field:

>>> chunk_group = client['config']['chunks'].aggregate([{ '$match': { 'ns': 'test.test2' }},{ '$group': { '_id': '$shard','nChunks': { '$sum': 1 }}}])
>>> print(', '.join(["%s: %i" % (ch['_id'], ch['nChunks']) for ch in chunk_group]))                                                                      
demo-set-0: 1

mtools/util/presplit.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

The pymongo related changes LGTM!

@stennie stennie merged commit b96568c into rueckstiess:develop Dec 17, 2021
@blink1073 blink1073 deleted the pymongo-4 branch December 17, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants