Skip to content

Commit ad580e0

Browse files
jskeetjskeet
committed
feat: All BatchGetDocuments RPCs to have customized retry settiings (per-FirestoreDb)
fix: Use FirestoreSettings.BatchGetDocuments for batch timing This will change the default timeout from 10 minutes to 5 minutes, but that will bring us in line with other clients. Any customer observing problems after this change can manually set the timeout in FirestoreSettings. I regard this as a "sufficiently safe" (but still *potentially* breaking) change to not merit a new major version. Fixes #11322.
1 parent 1531c7e commit ad580e0

4 files changed

Lines changed: 62 additions & 26 deletions

File tree

‎apis/Google.Cloud.Firestore/Google.Cloud.Firestore.Tests/FirestoreDbTest.cs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ public async Task GetDocumentSnapshotsAsync_UnknownResponseCase()
337337
await Assert.ThrowsAsync<InvalidOperationException>(() => db.GetDocumentSnapshotsAsync(new[] { db.Document("col1/doc1") }, transactionId: null, fieldMask: null, default));
338338
}
339339

340+
// This tests the default retry settings.
340341
[Fact]
341342
public async Task GetDocumentSnapshotsAsync_Retry()
342343
{
@@ -364,6 +365,37 @@ public async Task GetDocumentSnapshotsAsync_Retry()
364365
Assert.Equal(1, results.Count);
365366
}
366367

368+
// This is effectively just testing that custom retry settings are observed.
369+
[Fact]
370+
public async Task GetDocumentSnapshotsAsync_RetryProhibited()
371+
{
372+
var customRetrySettings = RetrySettings.FromConstantBackoff(maxAttempts: 1, backoff: TimeSpan.Zero, retryFilter: ex => true);
373+
374+
string docName = "projects/proj/databases/db/documents/col1/doc1";
375+
var mock = CreateMockClient();
376+
mock.Settings.Clock = new FakeClock();
377+
mock.Settings.Scheduler = new NoOpScheduler();
378+
379+
var request = new BatchGetDocumentsRequest
380+
{
381+
Database = "projects/proj/databases/db",
382+
Documents = { docName }
383+
};
384+
385+
var responses = new[]
386+
{
387+
new BatchGetDocumentsResponse { Missing = docName, ReadTime = CreateProtoTimestamp(1, 2) },
388+
};
389+
var errorResponses = responses.Where(r => throw new RpcException(new Status(StatusCode.Unavailable, "Bang")));
390+
mock.Configure().BatchGetDocuments(request, Arg.Any<CallSettings>())
391+
.Returns(new FakeDocumentStream(errorResponses), new FakeDocumentStream(errorResponses), new FakeDocumentStream(responses));
392+
var db = FirestoreDb.Create("proj", "db", mock, batchGetDocumentsRetrySettings: customRetrySettings);
393+
var docRef = db.Document("col1/doc1");
394+
395+
var exception = await Assert.ThrowsAsync<RpcException>(() => db.GetDocumentSnapshotsAsync(new[] { docRef }, transactionId: null, fieldMask: null, default));
396+
Assert.Equal("Bang", exception.Status.Detail);
397+
}
398+
367399
[Fact]
368400
public void WithWarningLogger()
369401
{

‎apis/Google.Cloud.Firestore/Google.Cloud.Firestore/FirestoreDb.cs‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ namespace Google.Cloud.Firestore
3131
/// </summary>
3232
public sealed class FirestoreDb
3333
{
34+
private static readonly RetrySettings s_defaultBatchGetDocumentsRetrySettings = RetrySettings.FromExponentialBackoff(
35+
maxAttempts: 5,
36+
initialBackoff: TimeSpan.FromMilliseconds(100),
37+
maxBackoff: TimeSpan.FromSeconds(60),
38+
backoffMultiplier: 1.3,
39+
retryFilter: RetrySettings.FilterForStatusCodes(StatusCode.Unavailable, StatusCode.Internal, StatusCode.DeadlineExceeded));
40+
3441
private const string DefaultDatabaseId = "(default)";
3542

3643
/// <summary>
@@ -62,9 +69,11 @@ public sealed class FirestoreDb
6269

6370
internal SerializationContext SerializationContext { get; }
6471

65-
private readonly CallSettings _batchGetCallSettings;
72+
private readonly RetrySettings _batchGetDocumentsRetrySettings;
6673

67-
private FirestoreDb(string projectId, string databaseId, FirestoreClient client, Action<string> warningLogger, SerializationContext serializationContext)
74+
private FirestoreDb(
75+
string projectId, string databaseId, FirestoreClient client, Action<string> warningLogger,
76+
SerializationContext serializationContext, RetrySettings batchGetDocumentsRetrySettings)
6877
{
6978
ProjectId = GaxPreconditions.CheckNotNull(projectId, nameof(projectId));
7079
DatabaseId = GaxPreconditions.CheckNotNull(databaseId, nameof(databaseId));
@@ -74,23 +83,10 @@ private FirestoreDb(string projectId, string databaseId, FirestoreClient client,
7483
DocumentsPath = $"{RootPath}/documents";
7584
WarningLogger = warningLogger;
7685

77-
// TODO: Potentially make these configurable.
78-
// The retry settings are taken from firestore_grpc_service_config.json.
79-
var batchGetRetry = RetrySettings.FromExponentialBackoff(
80-
maxAttempts: 5,
81-
initialBackoff: TimeSpan.FromMilliseconds(100),
82-
maxBackoff: TimeSpan.FromSeconds(60),
83-
backoffMultiplier: 1.3,
84-
retryFilter: RetrySettings.FilterForStatusCodes(StatusCode.Unavailable, StatusCode.Internal, StatusCode.DeadlineExceeded));
85-
_batchGetCallSettings = CallSettings.FromRetry(batchGetRetry).WithTimeout(TimeSpan.FromMinutes(10));
86-
8786
SerializationContext = GaxPreconditions.CheckNotNull(serializationContext, nameof(serializationContext));
88-
}
8987

90-
// Internally, we support non-default databases. The public Create and CreateAsync methods only support the default database,
91-
// as that's all the server supports at the moment. When that changes, we'll want to support non-default databases publicly,
92-
// but will probably need a different method name in order to do so, to avoid it being a breaking change.
93-
// We don't have a CreateAsync method accepting a database ID, as we don't use that anywhere for testing.
88+
_batchGetDocumentsRetrySettings = GaxPreconditions.CheckNotNull(batchGetDocumentsRetrySettings, nameof(batchGetDocumentsRetrySettings));
89+
}
9490

9591
/// <summary>
9692
/// Creates an instance for the specified project, using the specified <see cref="FirestoreClient"/> for RPC operations.
@@ -129,20 +125,23 @@ public static async Task<FirestoreDb> CreateAsync(string projectId = null, Fires
129125
/// <param name="client">The client to use for RPC operations. Must not be null.</param>
130126
/// <param name="warningLogger">The warning logger to use, if any. May be null.</param>
131127
/// <param name="converterRegistry">A registry of custom converters. May be null.</param>
128+
/// <param name="batchGetDocumentsRetrySettings">The retry settings for batch document fetching operations. May be null.</param>
132129
/// <returns>A new instance.</returns>
133130
internal static FirestoreDb Create(
134131
// Required parameters
135132
string projectId, string databaseId, FirestoreClient client,
136133
// Optional parameters
137134
Action<string> warningLogger = null,
138-
ConverterRegistry converterRegistry = null) =>
135+
ConverterRegistry converterRegistry = null,
136+
RetrySettings batchGetDocumentsRetrySettings = null) =>
139137
// Validation is performed in the constructor.
140138
new FirestoreDb(
141139
projectId,
142140
databaseId ?? DefaultDatabaseId,
143141
client,
144142
warningLogger,
145-
new SerializationContext(converterRegistry));
143+
new SerializationContext(converterRegistry),
144+
batchGetDocumentsRetrySettings ?? s_defaultBatchGetDocumentsRetrySettings);
146145

147146
/// <summary>
148147
/// Returns a new <see cref="FirestoreDb"/> with the same project, database and client as this one,
@@ -151,7 +150,7 @@ internal static FirestoreDb Create(
151150
/// <param name="warningLogger">The logger for warnings. May be null.</param>
152151
/// <returns>A new <see cref="FirestoreDb"/> based on this one, with the given warning logger.</returns>
153152
public FirestoreDb WithWarningLogger(Action<string> warningLogger) =>
154-
new FirestoreDb(ProjectId, DatabaseId, Client, warningLogger, SerializationContext);
153+
new FirestoreDb(ProjectId, DatabaseId, Client, warningLogger, SerializationContext, _batchGetDocumentsRetrySettings);
155154

156155
internal void LogWarning(string message) => WarningLogger?.Invoke(message);
157156

@@ -295,7 +294,7 @@ internal async Task<IList<DocumentSnapshot>> GetDocumentSnapshotsAsync(IEnumerab
295294

296295
var clock = Client.Settings.Clock ?? SystemClock.Instance;
297296
var scheduler = Client.Settings.Scheduler ?? SystemScheduler.Instance;
298-
var callSettings = _batchGetCallSettings.WithCancellationToken(cancellationToken);
297+
var callSettings = Client.Settings.BatchGetDocumentsSettings.WithCancellationToken(cancellationToken);
299298

300299
// This is the function that we'll retry. We can't use the built-in retry functionality, because it's not a unary gRPC call.
301300
// (We could potentially simulate a unary call, but it would be a little odd to do so.)
@@ -328,7 +327,7 @@ internal async Task<IList<DocumentSnapshot>> GetDocumentSnapshotsAsync(IEnumerab
328327
return snapshots;
329328
};
330329

331-
var retryingTask = RetryHelper.Retry(function, request, callSettings, clock, scheduler);
330+
var retryingTask = RetryHelper.Retry(_batchGetDocumentsRetrySettings, function, request, callSettings, clock, scheduler);
332331
return await retryingTask.ConfigureAwait(false);
333332

334333
string ExtractPath(DocumentReference documentReference)
@@ -387,7 +386,6 @@ public Task RunTransactionAsync(Func<Transaction, Task> callback, TransactionOpt
387386
/// <returns>A task which completes when the transaction has committed. The result of the task then contains the result of the callback.</returns>
388387
public async Task<T> RunTransactionAsync<T>(Func<Transaction, Task<T>> callback, TransactionOptions options = null, CancellationToken cancellationToken = default)
389388
{
390-
391389
ByteString previousTransactionId = null;
392390
options = options ?? TransactionOptions.Default;
393391
var attemptsLeft = options.MaxAttempts;

‎apis/Google.Cloud.Firestore/Google.Cloud.Firestore/FirestoreDbBuilder.cs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ public FirestoreDbBuilder() : base(FirestoreClient.ServiceMetadata)
4444
/// </summary>
4545
public FirestoreSettings Settings { get; set; }
4646

47+
/// <summary>
48+
/// The settings to use for BatchGetDocuments RPCs (used by all methods that get document snapshots),
49+
/// or null for the default settings.
50+
/// </summary>
51+
public RetrySettings BatchGetDocumentsRetrySettings { get; set; }
52+
4753
/// <summary>
4854
/// The ID of the Google Cloud project that contains the database. May be null, in which case
4955
/// the project will be automatically detected if possible.
@@ -137,7 +143,7 @@ protected override ChannelPool GetChannelPool() =>
137143
throw new InvalidOperationException($"This method should never execute in {nameof(FirestoreDbBuilder)}");
138144

139145
private FirestoreDb BuildFromClient(string projectId, FirestoreClient client) =>
140-
FirestoreDb.Create(projectId, DatabaseId, client, WarningLogger, ConverterRegistry);
146+
FirestoreDb.Create(projectId, DatabaseId, client, WarningLogger, ConverterRegistry, BatchGetDocumentsRetrySettings);
141147

142148
/// <summary>
143149
/// Returns the effective settings for a new client, including the "gccl" version header.

‎apis/Google.Cloud.Firestore/Google.Cloud.Firestore/RetryHelper.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2019, Google LLC
1+
// Copyright 2019, Google LLC
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -28,10 +28,10 @@ namespace Google.Cloud.Firestore
2828
internal static class RetryHelper
2929
{
3030
internal static async Task<TResponse> Retry<TRequest, TResponse>(
31+
RetrySettings retrySettings,
3132
Func<TRequest, CallSettings, Task<TResponse>> fn,
3233
TRequest request, CallSettings callSettings, IClock clock, IScheduler scheduler)
3334
{
34-
RetrySettings retrySettings = callSettings.Retry;
3535
if (retrySettings == null)
3636
{
3737
return await fn(request, callSettings).ConfigureAwait(false);

0 commit comments

Comments
 (0)