Skip to content

GODRIVER-3444 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors #1925

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

Merged
merged 26 commits into from
Apr 22, 2025

Conversation

prestonvasquez
Copy link
Member

@prestonvasquez prestonvasquez commented Jan 29, 2025

GODRIVER-3444

Summary

If maxAwaitTime option is set, use the min(maxAwaitTimeMS, remaining timeoutMS) as the maxTimeMS field on getMore commands.

Background & Motivation

From the DRIVERS-2868 proposal:

For tailable awaitData cursors we use the min(maxAwaitTimeMS, remaining timeoutMS - minRoundTripTime) to allow the server more opportunities to respond with an empty batch before a client-side timeout. Additionally, this change is
required to prevent an unnecessary client-side timeout during a pending read when checking out a connection. For
example, maxAwaitTimeMS=1000 and remaining timeoutMS=100 will cause a pending read to hang for 900ms.

Copy link
Contributor

mongodb-drivers-pr-bot bot commented Jan 29, 2025

API Change Report

No changes found!

@prestonvasquez prestonvasquez marked this pull request as draft February 27, 2025 18:10
@prestonvasquez prestonvasquez removed the request for review from matthewdale February 27, 2025 18:10
@@ -30,3 +37,35 @@ const (
UpdateOp = "update" // UpdateOp is the name for updating
BulkWriteOp = "bulkWrite" // BulkWriteOp is the name for client-level bulk write
)

func CalculateMaxTimeMS(ctx context.Context, rttMin time.Duration, rttStats string, err error) (int64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unconventional to pass the error to be returned in as a parameter. Instead, consider letting CalculateMaxTimeMS return just the int64 value, and let the caller form the error value.

E.g. use in batch_cursor.go:

maxTimeMS = driverutil.CalculateMaxTimeMS(ctx, rttMonitor.Min())
if maxTimeMS <= 0 {
	return nil, fmt.Errorf(
		"calculated server-side timeout (%v ms) is less than or equal to 0 (%v): %w",
		maxTimeMS,
		rttMonitor.Stats(),
		ErrDeadlineWouldBeExceeded)
}

@prestonvasquez prestonvasquez marked this pull request as ready for review April 3, 2025 02:02
@prestonvasquez prestonvasquez requested review from matthewdale and qingyang-hu and removed request for matthewdale April 3, 2025 02:02
@prestonvasquez prestonvasquez changed the title (POC) GODRIVER-3444 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors GODRIVER-3444 Adjust getMore maxTimeMS Calculation for tailable awaitData Cursors Apr 8, 2025
qingyang-hu
qingyang-hu previously approved these changes Apr 14, 2025
qingyang-hu
qingyang-hu previously approved these changes Apr 16, 2025
@prestonvasquez prestonvasquez requested a review from Copilot April 16, 2025 23:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

qingyang-hu
qingyang-hu previously approved these changes Apr 18, 2025
@prestonvasquez prestonvasquez merged commit d5c8b38 into mongodb:master Apr 22, 2025
31 of 34 checks passed
@prestonvasquez prestonvasquez deleted the GODRIVER-3444 branch April 22, 2025 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants