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.

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

@@ -337,6 +337,7 @@ public async Task GetDocumentSnapshotsAsync_UnknownResponseCase()
             await Assert.ThrowsAsync<InvalidOperationException>(() => db.GetDocumentSnapshotsAsync(new[] { db.Document("col1/doc1") }, transactionId: null, fieldMask: null, default));
         }
 
+        // This tests the default retry settings.
         [Fact]
         public async Task GetDocumentSnapshotsAsync_Retry()
         {
@@ -364,6 +365,37 @@ public async Task GetDocumentSnapshotsAsync_Retry()
             Assert.Equal(1, results.Count);
         }
 
+        // This is effectively just testing that custom retry settings are observed.
+        [Fact]
+        public async Task GetDocumentSnapshotsAsync_RetryProhibited()
+        {
+            var customRetrySettings = RetrySettings.FromConstantBackoff(maxAttempts: 1, backoff: TimeSpan.Zero, retryFilter: ex => true);
+
+            string docName = "projects/proj/databases/db/documents/col1/doc1";
+            var mock = CreateMockClient();
+            mock.Settings.Clock = new FakeClock();
+            mock.Settings.Scheduler = new NoOpScheduler();
+
+            var request = new BatchGetDocumentsRequest
+            {
+                Database = "projects/proj/databases/db",
+                Documents = { docName }
+            };
+
+            var responses = new[]
+            {
+                new BatchGetDocumentsResponse { Missing = docName, ReadTime = CreateProtoTimestamp(1, 2) },
+            };
+            var errorResponses = responses.Where(r => throw new RpcException(new Status(StatusCode.Unavailable, "Bang")));
+            mock.Configure().BatchGetDocuments(request, Arg.Any<CallSettings>())
+                .Returns(new FakeDocumentStream(errorResponses), new FakeDocumentStream(errorResponses), new FakeDocumentStream(responses));
+            var db = FirestoreDb.Create("proj", "db", mock, batchGetDocumentsRetrySettings: customRetrySettings);
+            var docRef = db.Document("col1/doc1");
+
+            var exception = await Assert.ThrowsAsync<RpcException>(() => db.GetDocumentSnapshotsAsync(new[] { docRef }, transactionId: null, fieldMask: null, default));
+            Assert.Equal("Bang", exception.Status.Detail);
+        }
+
         [Fact]
         public void WithWarningLogger()
         {

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

@@ -31,6 +31,13 @@ namespace Google.Cloud.Firestore
     /// </summary>
     public sealed class FirestoreDb
     {
+        private static readonly RetrySettings s_defaultBatchGetDocumentsRetrySettings = RetrySettings.FromExponentialBackoff(
+                maxAttempts: 5,
+                initialBackoff: TimeSpan.FromMilliseconds(100),
+                maxBackoff: TimeSpan.FromSeconds(60),
+                backoffMultiplier: 1.3,
+                retryFilter: RetrySettings.FilterForStatusCodes(StatusCode.Unavailable, StatusCode.Internal, StatusCode.DeadlineExceeded));
+
         private const string DefaultDatabaseId = "(default)";
 
         /// <summary>
@@ -62,9 +69,11 @@ public sealed class FirestoreDb
 
         internal SerializationContext SerializationContext { get; }
 
-        private readonly CallSettings _batchGetCallSettings;
+        private readonly RetrySettings _batchGetDocumentsRetrySettings;
 
-        private FirestoreDb(string projectId, string databaseId, FirestoreClient client, Action<string> warningLogger, SerializationContext serializationContext)
+        private FirestoreDb(
+            string projectId, string databaseId, FirestoreClient client, Action<string> warningLogger,
+            SerializationContext serializationContext, RetrySettings batchGetDocumentsRetrySettings)
         {
             ProjectId = GaxPreconditions.CheckNotNull(projectId, nameof(projectId));
             DatabaseId = GaxPreconditions.CheckNotNull(databaseId, nameof(databaseId));
@@ -74,23 +83,10 @@ private FirestoreDb(string projectId, string databaseId, FirestoreClient client,
             DocumentsPath = $"{RootPath}/documents";
             WarningLogger = warningLogger;
 
-            // TODO: Potentially make these configurable.
-            // The retry settings are taken from firestore_grpc_service_config.json.
-            var batchGetRetry = RetrySettings.FromExponentialBackoff(
-                maxAttempts: 5,
-                initialBackoff: TimeSpan.FromMilliseconds(100),
-                maxBackoff: TimeSpan.FromSeconds(60),
-                backoffMultiplier: 1.3,
-                retryFilter: RetrySettings.FilterForStatusCodes(StatusCode.Unavailable, StatusCode.Internal, StatusCode.DeadlineExceeded));
-            _batchGetCallSettings = CallSettings.FromRetry(batchGetRetry).WithTimeout(TimeSpan.FromMinutes(10));
-
             SerializationContext = GaxPreconditions.CheckNotNull(serializationContext, nameof(serializationContext));
-        }
 
-        // Internally, we support non-default databases. The public Create and CreateAsync methods only support the default database,
-        // as that's all the server supports at the moment. When that changes, we'll want to support non-default databases publicly,
-        // but will probably need a different method name in order to do so, to avoid it being a breaking change.
-        // We don't have a CreateAsync method accepting a database ID, as we don't use that anywhere for testing.
+            _batchGetDocumentsRetrySettings = GaxPreconditions.CheckNotNull(batchGetDocumentsRetrySettings, nameof(batchGetDocumentsRetrySettings));
+        }
 
         /// <summary>
         /// Creates an instance for the specified project, using the specified <see cref="FirestoreClient"/> for RPC operations.
@@ -129,20 +125,23 @@ private FirestoreDb(string projectId, string databaseId, FirestoreClient client,
         /// <param name="client">The client to use for RPC operations. Must not be null.</param>
         /// <param name="warningLogger">The warning logger to use, if any. May be null.</param>
         /// <param name="converterRegistry">A registry of custom converters. May be null.</param>
+        /// <param name="batchGetDocumentsRetrySettings">The retry settings for batch document fetching operations. May be null.</param>
         /// <returns>A new instance.</returns>
         internal static FirestoreDb Create(
             // Required parameters
             string projectId, string databaseId, FirestoreClient client,
             // Optional parameters
             Action<string> warningLogger = null,
-            ConverterRegistry converterRegistry = null) =>
+            ConverterRegistry converterRegistry = null,
+            RetrySettings batchGetDocumentsRetrySettings = null) =>
             // Validation is performed in the constructor.
             new FirestoreDb(
                 projectId,
                 databaseId ?? DefaultDatabaseId,
                 client,
                 warningLogger,
-                new SerializationContext(converterRegistry));
+                new SerializationContext(converterRegistry),
+                batchGetDocumentsRetrySettings ?? s_defaultBatchGetDocumentsRetrySettings);
 
         /// <summary>
         /// Returns a new <see cref="FirestoreDb"/> with the same project, database and client as this one,
@@ -151,7 +150,7 @@ private FirestoreDb(string projectId, string databaseId, FirestoreClient client,
         /// <param name="warningLogger">The logger for warnings. May be null.</param>
         /// <returns>A new <see cref="FirestoreDb"/> based on this one, with the given warning logger.</returns>
         public FirestoreDb WithWarningLogger(Action<string> warningLogger) =>
-            new FirestoreDb(ProjectId, DatabaseId, Client, warningLogger, SerializationContext);
+            new FirestoreDb(ProjectId, DatabaseId, Client, warningLogger, SerializationContext, _batchGetDocumentsRetrySettings);
 
         internal void LogWarning(string message) => WarningLogger?.Invoke(message);
 
@@ -295,7 +294,7 @@ internal async Task<IList<DocumentSnapshot>> GetDocumentSnapshotsAsync(IEnumerab
 
             var clock = Client.Settings.Clock ?? SystemClock.Instance;
             var scheduler = Client.Settings.Scheduler ?? SystemScheduler.Instance;
-            var callSettings = _batchGetCallSettings.WithCancellationToken(cancellationToken);
+            var callSettings = Client.Settings.BatchGetDocumentsSettings.WithCancellationToken(cancellationToken);
 
             // 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.
             // (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
                 return snapshots;
             };
 
-            var retryingTask = RetryHelper.Retry(function, request, callSettings, clock, scheduler);
+            var retryingTask = RetryHelper.Retry(_batchGetDocumentsRetrySettings, function, request, callSettings, clock, scheduler);
             return await retryingTask.ConfigureAwait(false);
 
             string ExtractPath(DocumentReference documentReference)
@@ -387,7 +386,6 @@ public Task RunTransactionAsync(Func<Transaction, Task> callback, TransactionOpt
         /// <returns>A task which completes when the transaction has committed. The result of the task then contains the result of the callback.</returns>
         public async Task<T> RunTransactionAsync<T>(Func<Transaction, Task<T>> callback, TransactionOptions options = null, CancellationToken cancellationToken = default)
         {
-
             ByteString previousTransactionId = null;
             options = options ?? TransactionOptions.Default;
             var attemptsLeft = options.MaxAttempts;

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

@@ -44,6 +44,12 @@ public FirestoreDbBuilder() : base(FirestoreClient.ServiceMetadata)
         /// </summary>
         public FirestoreSettings Settings { get; set; }
 
+        /// <summary>
+        /// The settings to use for BatchGetDocuments RPCs (used by all methods that get document snapshots),
+        /// or null for the default settings.
+        /// </summary>
+        public RetrySettings BatchGetDocumentsRetrySettings { get; set; }
+
         /// <summary>
         /// The ID of the Google Cloud project that contains the database. May be null, in which case
         /// the project will be automatically detected if possible.
@@ -137,7 +143,7 @@ public override async Task<FirestoreDb> BuildAsync(CancellationToken cancellatio
             throw new InvalidOperationException($"This method should never execute in {nameof(FirestoreDbBuilder)}");
 
         private FirestoreDb BuildFromClient(string projectId, FirestoreClient client) =>
-            FirestoreDb.Create(projectId, DatabaseId, client, WarningLogger, ConverterRegistry);
+            FirestoreDb.Create(projectId, DatabaseId, client, WarningLogger, ConverterRegistry, BatchGetDocumentsRetrySettings);
 
         /// <summary>
         /// Returns the effective settings for a new client, including the "gccl" version header.

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

@@ -1,4 +1,4 @@
-// Copyright 2019, Google LLC
+// Copyright 2019, Google LLC
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -28,10 +28,10 @@ namespace Google.Cloud.Firestore
     internal static class RetryHelper
     {
         internal static async Task<TResponse> Retry<TRequest, TResponse>(
+            RetrySettings retrySettings,
             Func<TRequest, CallSettings, Task<TResponse>> fn,
             TRequest request, CallSettings callSettings, IClock clock, IScheduler scheduler)
         {
-            RetrySettings retrySettings = callSettings.Retry;
             if (retrySettings == null)
             {
                 return await fn(request, callSettings).ConfigureAwait(false);