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

PYTHON-4227 Unified tests: Advance cluster_time of ClientSessions after initialData creation #1603

Conversation

caseyclements
Copy link
Contributor

Changes

After initialData is added in test.unified_format's run_scenario, we get the $clusterTime from the server, and add this to each of the ClientSession entities before further operations.

Motivation for the change.

DRIVERS-2816

Here is the pertinent section of the specifications

@caseyclements caseyclements requested a review from a team as a code owner April 17, 2024 15:59
@caseyclements
Copy link
Contributor Author

I'm investigating the new failure that popped up. I can reproduce locally.

FAILURE: pymongo.errors.OperationFailure: PlanExecutor error during aggregation :: caused by :: indexes of target collection db0.coll0 changed during processing., 

self = <test.test_crud_unified.TestUnifiedDbAggregateWriteReadPreference testMethod=test_Database-level_aggregate_with_$out_includes_read_preference_for_5_0+_server>

@blink1073
Copy link
Member

That's https://jira.mongodb.org/browse/PYTHON-4356

@ShaneHarvey ShaneHarvey changed the title [PYTHON-4227] Unified tests: Advance cluster_time of ClientSessions after initialData creation PYTHON-4227 Unified tests: Advance cluster_time of ClientSessions after initialData creation Apr 17, 2024
blink1073
blink1073 previously approved these changes Apr 17, 2024
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

if cluster_time:
for entity in self.entity_map._entities.values():
if isinstance(entity, ClientSession):
entity.advance_cluster_time(cluster_time)
Copy link
Member

Choose a reason for hiding this comment

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

We need to do this for sessions created later on in _testOperation_createEntities too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is _testOperation_createEntities called from? Would we want to ping again before updating?

Copy link
Member

Choose a reason for hiding this comment

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

We should only run ping once, store the cluster_time, then use it later when _testOperation_createEntities is called.

# to ensure consistency in transactions against a sharded deployment,
cluster_time = self.client.admin.command("ping").get("$clusterTime")
if cluster_time:
for entity in self.entity_map._entities.values():
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding a EntityMapUtil.values() method that we can call here so we don't need to look at the internals.

Copy link
Member

@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.

LGTM once the typing issue is fixed.

@caseyclements
Copy link
Contributor Author

@ShaneHarvey, @blink1073 test.test_transactions_unified.TestUnifiedMongosUnpin.test_unpin_after_TransientTransactionError_error_on_abort is still failing intermittently. This PR had removed the skip. Should I put it back on?

@blink1073
Copy link
Member

Should I put it back on?

Yeah, apparently https://jira.mongodb.org/browse/PYTHON-4182 was unrelated, I unblocked it and put it in scheduled.

blink1073
blink1073 previously approved these changes Apr 23, 2024
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

"""Manually synchronize entities when desired"""
if not self._cluster_time:
self._cluster_time = self.test.client.admin.command("ping").get("$clusterTime")
for entity in self._entities:
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
for entity in self._entities:
for entity in self._entities.values():

The current version iterates over the names instead of the values and never passes the isinstance check.

if "unpin after non-transient error on abort" in spec["description"]:
if client_context.version[0] == 8:
self.skipTest("Skipping TransientTransactionError pending PYTHON-4182")
if "unpin after TransientTransactionError error on" in spec["description"]:
Copy link
Contributor

@NoahStapp NoahStapp Apr 23, 2024

Choose a reason for hiding this comment

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

Is this intended? The failure is the same for PYTHON-4182, so I would expect this PR to fix that ticket as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try it out. The failure was popping back up earlier but now that everything is cleaner, and I've synced everything up to master, it is worth trying again.

@caseyclements caseyclements force-pushed the PYTHON-4227-advance-clustertime-in-unified-tests branch from 2afe41b to 8bc865b Compare April 23, 2024 21:49
@blink1073
Copy link
Member

Merging so we can check the waterfall afterward.

@blink1073 blink1073 merged commit e8900ad into mongodb:master Apr 24, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants