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
fix: set timeouts for BatchGetDocuments/RunQuery #799
Conversation
70b78b1
to
e32a6c7
Compare
FirestoreOptions.newBuilder() | ||
.setRetrySettings( | ||
RetrySettings.newBuilder() | ||
.setMaxRpcTimeout(Duration.ofMillis(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to add some asserts to the retry settings once built in a unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the code in the stub handler is not exercised by unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, can we add one? Something along the lines of:
FirestoreOptions firestoreOptions =
FirestoreOptions.newBuilder()
.setRetrySettings(
RetrySettings.newBuilder()
.setMaxRpcTimeout(Duration.ofMillis(1))
.setTotalTimeout(Duration.ofMillis(1))
.setInitialRpcTimeout(Duration.ofMillis(1))
.build())
.build();
Duration maxRpcTimeout = firestoreOptions.getRetrySettings().getMaxRpcTimeout();
Truth.assertThat(maxRpcTimeout).isEqualTo(Duration.ofMillis(1));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and could add additional cases for the unary timeouts that were already getting set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this would not catch the issue here. The issue is that the retry settings do not get applied correctly to the underlying RCPs, not that the settings itself are off. The code snippet you provided is just auto-generated code (unless I misunderstood the intent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it - I thought FirestoreOptions was handwritten. SGTM
We currently only apply the retry settings to Unary methods, which means that the user cannot specify a custom timeout for BatchGetDocuments and RunQuery requests.