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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敶 Transactions are not used now! #86

Closed
Olegt0rr opened this issue Dec 22, 2020 · 8 comments 路 May be fixed by #117
Closed

馃敶 Transactions are not used now! #86

Olegt0rr opened this issue Dec 22, 2020 · 8 comments 路 May be fixed by #117
Labels
bug Something isn't working

Comments

@Olegt0rr
Copy link
Contributor

Olegt0rr commented Dec 22, 2020

Bug

In the _save() method transaction's session is not passed to update_one() request.

odmantic/odmantic/engine.py

Lines 315 to 320 in 541d411

await collection.update_one(
{"_id": getattr(instance, instance.__primary_field__)},
{"$set": doc},
upsert=True,
bypass_document_validation=True,
)

It means that transaction is NOT USED NOW!

How to fix?

It's not enough to just pass the session to request.
In MongoDB transactions are not available for Standalone.
This simple way Standalone users will get exceptions for every save() call 馃槩

So we need to define the users of Standalone version and not start a transaction for them.
Example:

if self.client._server_property('server_type') == SERVER_TYPE.Standalone:
    await self._save(instance, None, **kwargs)
else:
    async with await self.client.start_session() as s:
        async with s.start_transaction():
            await self._save(instance, s, **kwargs)

P.S.: I don't know how to properly check ServerType without protected _server_property method :(

@Olegt0rr Olegt0rr added the bug Something isn't working label Dec 22, 2020
@Olegt0rr Olegt0rr changed the title 馃敶 Transactions is not used now! 馃敶 Transactions is not used now! Dec 22, 2020
@Olegt0rr Olegt0rr changed the title 馃敶 Transactions is not used now! 馃敶 Transactions are not used now! Dec 22, 2020
@art049
Copy link
Owner

art049 commented Dec 24, 2020

Nice catch @Olegt0rr 馃

In MongoDB transactions are not available for Standalone.

Do you have some references about this ?
The only transaction requirements I found were those https://docs.mongodb.com/manual/core/write-operations-atomicity/
Edit: Actually, it's stated that this would only work on replicaSets

@art049
Copy link
Owner

art049 commented Dec 24, 2020

To find out if we're connected to a replicaset or not, i found another way, using the db.serverStatus command:

server_status = await self.database.command({'serverStatus'  :1})
is_standalone = "repl" not in server_status

I tried to use the _server_property you describe but I couldn't find it on my local client.
It would be probably faster to use it as it would avoid to send an extra request to the database.

@Olegt0rr
Copy link
Contributor Author

Olegt0rr commented Dec 25, 2020

Do you have some references about this ?

https://docs.mongodb.com/manual/core/transactions-production-consideration/#feature-compatibility

Replica Set (4.0+) and Sharded Cluster (4.2+) support transactions.
Standalone is not listed here.
Also you can run an experiment: pass session to request and your PR tester will answer with error 馃槩


i found another way, using the db.serverStatus command

This way requires server admin role:
not authorized on admin to execute command

For self.client._server_property('server_type') - readWrite role is enough.
I've found it in pymongo similar solution:
https://github.com/mongodb/mongo-python-driver/blob/6b0123594aeb6d4fa618568a28cab3344c5422f0/pymongo/mongo_client.py#L1033-L1039

Also added request to MongoDB jira: https://jira.mongodb.org/browse/PYTHON-2476

@art049
Copy link
Owner

art049 commented Dec 27, 2020

This way requires server admin role:
not authorized on admin to execute command

Yes 馃槶, I totally forgot this initial constraint.

Thanks for creating the request in Jira !
I just saw an anwser there:

Bernie Hackett added a comment - Dec 25 2020 03:30:26 PM GMT+0000
The easiest way to do what you want, with no API changes, is to call the ismaster command. If the msg field is set to "isdbgrid" you're connected to a mongos. If the setname field is set you're connected to a replica set memeber. Otherwise you're connected to a standalone.

This looks like a very good solution. We will have to check the version as well to make sure sharded cluster are 4.2+.
To add these commands in the Engine initializer, we might have to gather the event loop held by the motor client or maybe access the underlying pymongo client (I'm not sure it's doable in a clean way for this one)

@Olegt0rr
Copy link
Contributor Author

Seems calling self.client._server_property('server_type') still easiest way 馃槶

@Olegt0rr
Copy link
Contributor Author

Need some help to finish tests coverage

@art049
Copy link
Owner

art049 commented Feb 2, 2021

Hey, yes I will merge #91 as soon as possible, this should help you with the coverage :)

@art049
Copy link
Owner

art049 commented Aug 23, 2022

Closed by #227

@art049 art049 closed this as completed Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants