Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants