fix: set timeouts for BatchGetDocuments/RunQuery #799
Conversation
product-auto-label
bot
added
the
api: firestore
google-cla
bot
added
the
cla: yes
schmidt-sebastian
force-pushed
the
mrschmidt/timeout
branch
from
70b78b1 to
e32a6c7
Compare
schmidt-sebastian
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| FirestoreOptions.newBuilder() | ||
| .setRetrySettings( | ||
| RetrySettings.newBuilder() | ||
| .setMaxRpcTimeout(Duration.ofMillis(1)) |
There was a problem hiding this comment.
might be nice to add some asserts to the retry settings once built in a unit test?
There was a problem hiding this comment.
Unfortunately, the code in the stub handler is not exercised by unit tests.
There was a problem hiding this comment.
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.
and could add additional cases for the unary timeouts that were already getting set.
There was a problem hiding this comment.
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.
Got it - I thought FirestoreOptions was handwritten. SGTM
3 participants
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.