Skip to content

Commit 6ffc5a5

Browse files
committed
chore: Resolve review comments, make session refresh only one method
chore: Remove Task.Run for creating the session
1 parent 25bc8ed commit 6ffc5a5

File tree

4 files changed

+37
-65
lines changed

4 files changed

+37
-65
lines changed

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/V1/DirectedReadTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ private Task<ManagedTransaction> CreateManagedTransaction(SpannerClient client,
340340
Multiplexed = true
341341
};
342342

343-
return managedSession.CreateManagedTransactionWithSpannerTransaction(transactionId, options, singleUseTransaction: singleUse);
343+
return managedSession.CreateManagedTransaction(transactionId, options, singleUseTransaction: singleUse);
344344
}
345345

346346
public class FakeGrpcSpannerClient : V1.Spanner.SpannerClient

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/V1/RouteToLeaderTests.cs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,20 +122,20 @@ public async Task SpannerClient_DoesNotRouteToLeaderWhenNotEnabled(Func<SpannerC
122122

123123
public static TheoryData<Func<ManagedSession, Task>> ManagedTransactionRoutesToLeader => new TheoryData<Func<ManagedSession, Task>>
124124
{
125-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_partitionedDml, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
126-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
127-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_partitionedDml, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
128-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readWrite, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
129-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_partitionedDml, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
130-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
131-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
125+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_partitionedDml, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
126+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
127+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_partitionedDml, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
128+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readWrite, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
129+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_partitionedDml, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
130+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
131+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readWrite, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
132132
};
133133

134134
public static TheoryData<Func<ManagedSession, Task>> ManagedTransactionDoesNotRouteToLeader => new TheoryData<Func<ManagedSession, Task>>
135135
{
136-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readOnly, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
137-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readOnly, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
138-
{ async managedSession => await (await managedSession.CreateManagedTransactionWithSpannerTransaction(s_transactionId, s_readOnly, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
136+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readOnly, false)).ExecuteSqlAsync(new ExecuteSqlRequest(), callSettings: null) },
137+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readOnly, false)).ReadStreamReaderAsync(new ReadRequest(), callSettings: null).NextAsync(default) },
138+
{ async managedSession => await (await managedSession.CreateManagedTransaction(s_transactionId, s_readOnly, false)).ExecuteSqlStreamReaderAsync(new ExecuteSqlRequest(), callSettings: null).NextAsync(default) },
139139
};
140140

141141
[Theory]

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/ManagedSession.cs

Lines changed: 23 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -27,19 +27,24 @@
2727
namespace Google.Cloud.Spanner.V1;
2828

2929
/// <summary>
30-
/// TODO: Add summary for mux sessions
30+
/// This class helps manage Spanner multiplex session creation and lifecycle.
31+
/// A <see cref="ManagedSession"/> should only be created through a <see cref="SessionBuilder"/>.
32+
/// It provides factory methods to create <see cref="ManagedTransaction"/> which will be created using the current underlying session in this ManagedSession.
33+
/// A check to see if the underlying session needs to be refreshed or created will be done everytime a method to create a ManagedTransaction is called.
34+
/// The class will do a soft, non-blocking refresh if the underlying session is greater than 7 days but less than 28 days old from its creation.
35+
/// A hard, blocking refresh is done if the session is greater than 28 days old from its creation.
3136
/// </summary>
3237
public class ManagedSession
3338
{
39+
private const double HardRefreshIntervalInDays = 28.0;
40+
private const double SoftRefreshIntervalInDays = 7.0;
41+
3442
private readonly Logger _logger;
3543
private readonly CreateSessionRequest _createSessionRequestTemplate;
3644
private Task _sessionCreationTask;
3745
private readonly object _sessionCreationTaskLock = new object();
3846

39-
internal Session _session;
40-
41-
private const double HardRefreshIntervalInDays = 28.0;
42-
private const double SoftRefreshIntervalInDays = 7.0;
47+
private Session _session;
4348

4449
private readonly IClock _clock;
4550

@@ -48,15 +53,13 @@ public class ManagedSession
4853
/// </summary>
4954
internal SpannerClient Client { get; }
5055

51-
internal Task<Session> CreateSessionTask { get; }
52-
5356
/// <summary>
5457
/// The name of the session. This is never null.
5558
/// </summary>
5659
internal SessionName SessionName => Session.SessionName;
5760

5861
/// <summary>
59-
/// The Spanner session resource associated to this pooled session.
62+
/// The Spanner session resource associated to this ManagedSession.
6063
/// Won't be null.
6164
/// </summary>
6265
internal Session Session
@@ -87,8 +90,9 @@ internal Session Session
8790
/// <param name="dbName"></param>
8891
/// <param name="dbRole"></param>
8992
/// <param name="options"></param>
90-
public ManagedSession(SpannerClient client, DatabaseName dbName, string dbRole, ManagedSessionOptions options)
93+
internal ManagedSession(SpannerClient client, DatabaseName dbName, string dbRole, ManagedSessionOptions options)
9194
{
95+
GaxPreconditions.CheckNotNull(dbName, nameof(dbName));
9296
Client = GaxPreconditions.CheckNotNull(client, nameof(client));
9397
Options = options ?? new ManagedSessionOptions();
9498
_logger = client.Settings.Logger; // Just to avoid fetching it all the time
@@ -112,26 +116,26 @@ public ManagedSession(SpannerClient client, DatabaseName dbName, string dbRole,
112116
/// <summary>
113117
/// Returns a ManagedTransaction for the same ManagedSession as this with the given <paramref name="transactionOptions"/>.
114118
/// </summary>
115-
public async Task<ManagedTransaction> CreateManagedTransactionWithOptions(TransactionOptions transactionOptions, bool singleUseTransaction)
119+
public async Task<ManagedTransaction> CreateManagedTransaction(TransactionOptions transactionOptions, bool singleUseTransaction, CancellationToken cancellationToken = default)
116120
{
117-
await MaybeRefreshWithTimePeriodCheck().ConfigureAwait(false);
121+
await CreateOrRefreshSessionsAsync(cancellationToken).ConfigureAwait(false);
118122
return new ManagedTransaction(this, transactionId: null, transactionOptions, singleUseTransaction, readTimestamp: null);
119123
}
120124

121125
/// <summary>
122126
/// Returns a ManagedTransaction for the same ManagedSession as this one with the given transaction related values.
123127
/// </summary>
124-
public async Task<ManagedTransaction> CreateManagedTransactionWithSpannerTransaction(ByteString transactionId, TransactionOptions transactionOptions, bool singleUseTransaction, Timestamp readTimestamp = null)
128+
public async Task<ManagedTransaction> CreateManagedTransaction(ByteString transactionId, TransactionOptions transactionOptions, bool singleUseTransaction, Timestamp readTimestamp = null, CancellationToken cancellationToken = default)
125129
{
126130
GaxPreconditions.CheckNotNull(transactionId, nameof(transactionId));
127-
await MaybeRefreshWithTimePeriodCheck().ConfigureAwait(false);
131+
await CreateOrRefreshSessionsAsync(cancellationToken).ConfigureAwait(false);
128132
return new ManagedTransaction(this, transactionId, transactionOptions, singleUseTransaction, readTimestamp);
129133
}
130134

131135
/// <summary>
132136
/// Returns a ManagedTransaction for the same ManagedSession as this one with the given transaction mode.
133137
/// </summary>
134-
public async Task<ManagedTransaction> CreateManagedTransactionWithMode(ByteString transactionId, ModeOneofCase transactionMode, Timestamp readTimestamp = null)
138+
public async Task<ManagedTransaction> CreateManagedTransaction(ByteString transactionId, ModeOneofCase transactionMode, Timestamp readTimestamp = null)
135139
{
136140
TransactionOptions BuildTransactionOptions() => transactionMode switch
137141
{
@@ -142,42 +146,7 @@ public async Task<ManagedTransaction> CreateManagedTransactionWithMode(ByteStrin
142146
_ => throw new ArgumentException(nameof(transactionMode), $"Unknown {typeof(ModeOneofCase).FullName}: {transactionMode}")
143147
};
144148

145-
return await CreateManagedTransactionWithSpannerTransaction(transactionId, BuildTransactionOptions(), false, readTimestamp).ConfigureAwait(false);
146-
}
147-
148-
private async Task<bool> UpdateMuxSession(bool needsRefresh, double intervalInDays)
149-
{
150-
Session oldSession = _session;
151-
await CreateOrRefreshSessionsAsync(default).ConfigureAwait(false);
152-
153-
return _session != oldSession;
154-
}
155-
156-
internal async Task MaybeRefreshWithTimePeriodCheck()
157-
{
158-
if (SessionHasExpired(HardRefreshIntervalInDays))
159-
{
160-
// If the session has expired on a client RPC request call, or has exceeded the 28 day Mux session refresh guidance
161-
// No request can proceed without us having a new Session to work with
162-
// Block on refreshing and getting a new session
163-
bool sessionIsRefreshed = await UpdateMuxSession(true, HardRefreshIntervalInDays).ConfigureAwait(false);
164-
165-
if (!sessionIsRefreshed)
166-
{
167-
throw new Exception("Unable to refresh multiplex session, and the old session has expired or is 28 days past refresh");
168-
}
169-
170-
_logger.Info($"Refreshed session since it was expired or past 28 days refresh period. New session {SessionName}");
171-
}
172-
173-
if (SessionHasExpired(SoftRefreshIntervalInDays))
174-
{
175-
// The Mux sessions have a lifespan of 28 days. We check if we need a session refresh in every request needing the session
176-
// If the timespan between a request needing a session and the session creation time is greater than 7 days, we proactively refresh the mux session
177-
// The request can safely use the older session since it is still valid while we do this refresh to fetch the new session.
178-
// Hence fire and forget the session refresh.
179-
_ = Task.Run(() => UpdateMuxSession(true, SoftRefreshIntervalInDays));
180-
}
149+
return await CreateManagedTransaction(transactionId, BuildTransactionOptions(), false, readTimestamp).ConfigureAwait(false);
181150
}
182151

183152
// internal for testing
@@ -218,7 +187,7 @@ private async Task CreateOrRefreshSessionsAsync(CancellationToken cancellationTo
218187
return;
219188
}
220189

221-
// Hard refresh or initial session creation
190+
// Hard refresh or initial session creation,
222191
Task currentCreationTask = TriggerRefresh(HardRefreshIntervalInDays);
223192

224193
// 2b. Block the current caller on the task (Hard Refresh requirement)
@@ -317,12 +286,13 @@ public SessionBuilder(DatabaseName databaseName, SpannerClient client)
317286
public ManagedSessionOptions Options { get; set; }
318287

319288
/// <summary>
320-
/// The database for this managed session
289+
/// The <see cref="DatabaseName"/> for this managed session.
290+
/// This is a required field and will be used when creating the underlying Spanner session.
321291
/// </summary>
322292
public DatabaseName DatabaseName { get; set; }
323293

324294
/// <summary>
325-
/// The database role of the managed session
295+
/// The database role of the managed session. This will be used when creating the underlying Spanner session.
326296
/// </summary>
327297
public string DatabaseRole { get; set; }
328298

apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/ManagedSessionOptions.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,15 @@ namespace Google.Cloud.Spanner.V1;
2121
/// </summary>
2222
public class ManagedSessionOptions
2323
{
24-
private TimeSpan _timeout = TimeSpan.FromSeconds(60);
24+
private const double DefaultTimeoutSeconds = 60.0;
25+
private TimeSpan _timeout;
2526

2627
/// <summary>
2728
/// Constructs a new <see cref="ManagedSessionOptions"/> with default values.
2829
/// </summary>
2930
public ManagedSessionOptions()
3031
{
32+
_timeout = TimeSpan.FromSeconds(DefaultTimeoutSeconds);
3133
}
3234

3335
/// <summary>

0 commit comments

Comments
 (0)