Skip to content

Conversation

NoahStapp
Copy link
Contributor

No description provided.

@NoahStapp NoahStapp requested a review from sleepyStick January 29, 2025 21:04
@NoahStapp
Copy link
Contributor Author

The remaining failure is
test.asynchronous.test_concurrency.TestAsyncConcurrency.test_concurrency, which is a known flakey test that depends on network and platform conditions.

sleepyStick
sleepyStick previously approved these changes Jan 30, 2025
db = self.db

# Start Aggregation Example 1

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't matter that much, but new line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

sleepyStick
sleepyStick previously approved these changes Jan 30, 2025
@NoahStapp NoahStapp requested a review from ShaneHarvey January 30, 2025 21:12
session=s,
)
).next()
)["adoptableCatsCount"]
Copy link
Member

Choose a reason for hiding this comment

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

Is this really ruff's formatting? Ew.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's not great.

db.inventory.insert_one({"username": "alice"})
db.inventory.delete_one({"username": "alice"})

t = asyncio.create_task(insert_docs())
Copy link
Member

Choose a reason for hiding this comment

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

Could we utilize a Thread/Task wrapper api to avoid duplicating tests like this? For example, like ExceptionCatchingTask/ExceptionCatchingThread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to how SpecRunnerThread/SpecRunnerTask work? We could move that abstraction up to be a generic class we use everywhere we need a similar pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated #2094 for this use case.

@NoahStapp NoahStapp requested a review from ShaneHarvey January 31, 2025 18:44
@NoahStapp NoahStapp requested a review from sleepyStick February 4, 2025 18:19
sleepyStick
sleepyStick previously approved these changes Feb 4, 2025
if self.args:
await self.target(*self.args)
else:
await self.target()
Copy link
Member

Choose a reason for hiding this comment

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

await self.target(*self.args) will work even if self.args is an empty list so this code can be simplified.

return not self.stopped

async def run(self):
if self.target:
Copy link
Member

Choose a reason for hiding this comment

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

When would "target" ever be None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we'd want it to throw an error here instead of being a silent no-op.

nonlocal done
while not done:
db.inventory.insert_one({"username": "alice"})
db.inventory.delete_one({"username": "alice"})
Copy link
Member

@ShaneHarvey ShaneHarvey Feb 4, 2025

Choose a reason for hiding this comment

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

We should probably put a short sleep in here (5ms maybe) to avoid a tight loop.

@NoahStapp NoahStapp requested a review from ShaneHarvey February 4, 2025 19:47
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.

Sorry one last question.

if self.target:
await self.target()
await self.target(*self.args)
self.stopped = True
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set stopped to try even when target raises? If so I suggest using try/finally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'd say so.

@NoahStapp NoahStapp requested a review from ShaneHarvey February 4, 2025 20:59
@NoahStapp NoahStapp merged commit 7a4150a into mongodb:master Feb 5, 2025
34 of 46 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

Development

Successfully merging this pull request may close these issues.

3 participants