Skip to content

Commit

Permalink
fix: address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@olavloite @jskeet
olavloite authored and jskeet committed Jul 21, 2021
1 parent d2025be commit d26b04c
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 33 deletions.
26 changes: 5 additions & 21 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SessionPoolManagerTests.cs
View file
Expand Up @@ -38,7 +38,7 @@ public async Task EqualOptions_SameSessionPool()
factoryCalls++;
return Task.FromResult<SpannerClient>(new FailingSpannerClient());
};
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.DefaultSpannerSettings(), Logger.DefaultLogger, factory);
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.CreateDefaultSpannerSettings(), Logger.DefaultLogger, factory);

var options1 = new SpannerClientCreationOptions(new SpannerConnectionStringBuilder(ConnectionString));
var options2 = new SpannerClientCreationOptions(new SpannerConnectionStringBuilder(ConnectionString));
Expand All @@ -59,7 +59,7 @@ public async Task DifferentOptions_DifferentSessionPools()
factoryCalls++;
return Task.FromResult<SpannerClient>(new FailingSpannerClient());
};
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.DefaultSpannerSettings(), Logger.DefaultLogger, factory);
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.CreateDefaultSpannerSettings(), Logger.DefaultLogger, factory);

var options1 = new SpannerClientCreationOptions(new SpannerConnectionStringBuilder(ConnectionString));
var options2 = new SpannerClientCreationOptions(new SpannerConnectionStringBuilder(ConnectionString) { Port = 1234 });
Expand Down Expand Up @@ -92,34 +92,18 @@ public void Create_UsesDefaultLogger()
Assert.Same(Logger.DefaultLogger, manager.Logger);
}

[Fact]
public void CreateWithNullSettings_UsesDefaultLogger()
{
var manager = SessionPoolManager.Create(new SessionPoolOptions(), null, null);
Assert.Same(Logger.DefaultLogger, manager.Logger);
}

[Fact]
public void CreateWithSettings_UsesLoggerInSettings()
{
var settings = new SpannerSettings {Logger = new DefaultLogger()};
var manager = SessionPoolManager.Create(new SessionPoolOptions(), settings);
var manager = SessionPoolManager.CreateWithSettings(new SessionPoolOptions(), settings);
Assert.Same(settings.Logger, manager.Logger);
}

[Fact]
public void CreateWithSettingsAndLogger_UsesLogger()
{
var settings = new SpannerSettings{Logger = new DefaultLogger()};
var logger = new DefaultLogger();
var manager = SessionPoolManager.Create(new SessionPoolOptions(), settings, logger);
Assert.Same(logger, manager.Logger);
}

[Fact]
public async Task ReleaseDecreasesCount()
{
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.DefaultSpannerSettings(), Logger.DefaultLogger, FailingSpannerClient.Factory);
var manager = new SessionPoolManager(new SessionPoolOptions(), SessionPoolManager.CreateDefaultSpannerSettings(), Logger.DefaultLogger, FailingSpannerClient.Factory);

var options = new SpannerClientCreationOptions(new SpannerConnectionStringBuilder(ConnectionString));
var pool = await manager.AcquireSessionPoolAsync(options);
Expand All @@ -141,7 +125,7 @@ public async Task ReleaseDecreasesCount()
public async Task EmulatorDetection_AlwaysUsesRegularOptions(string emulatorHost)
{
var regularOptions = new SessionPoolOptions();
var manager = new SessionPoolManager(regularOptions, SessionPoolManager.DefaultSpannerSettings(), Logger.DefaultLogger, FailingSpannerClient.Factory);
var manager = new SessionPoolManager(regularOptions, SessionPoolManager.CreateDefaultSpannerSettings(), Logger.DefaultLogger, FailingSpannerClient.Factory);

var builder = new SpannerConnectionStringBuilder(ConnectionString)
{
Expand Down
21 changes: 9 additions & 12 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SessionPoolManager.cs
View file
Expand Up @@ -50,7 +50,7 @@ static SessionPoolManager()
/// is specified on construction.
/// </summary>
public static SessionPoolManager Default { get; } =
new SessionPoolManager(new SessionPoolOptions(), DefaultSpannerSettings(), Logger.DefaultLogger, CreateClientAsync);
new SessionPoolManager(new SessionPoolOptions(), CreateDefaultSpannerSettings(), Logger.DefaultLogger, CreateClientAsync);

private readonly Func<SpannerClientCreationOptions, SpannerSettings, Logger, Task<SpannerClient>> _clientFactory;

Expand All @@ -71,18 +71,17 @@ static SessionPoolManager()

/// <summary>
/// The SpannerSettings used by this SessionPoolManager. These are expected to remain unaltered for the lifetime of the manager.
/// Currently the default settings are used in all cases, but with the "gccl" version header added to specify the version of Google.Cloud.Spanner.Data
/// being used.
/// The "gccl" version header is added to the SpannerSettings to specify the version of Google.Cloud.Spanner.Data being used.
/// </summary>
internal SpannerSettings SpannerSettings { get; }

private static SpannerSettings AppendVersionHeader(SpannerSettings settings)
private static SpannerSettings AppendAssemblyVersionHeader(SpannerSettings settings)
{
settings.VersionHeaderBuilder.AppendAssemblyVersion("gccl", typeof(SessionPoolManager));
return settings;
}

internal static SpannerSettings DefaultSpannerSettings() => new SpannerSettings();
internal static SpannerSettings CreateDefaultSpannerSettings() => new SpannerSettings();

/// <summary>
/// Constructor for test purposes, allowing the SpannerClient creation to be customized (e.g. for
Expand All @@ -99,7 +98,7 @@ private static SpannerSettings AppendVersionHeader(SpannerSettings settings)
Func<SpannerClientCreationOptions, SpannerSettings, Logger, Task<SpannerClient>> clientFactory)
{
SessionPoolOptions = GaxPreconditions.CheckNotNull(options, nameof(options));
SpannerSettings = AppendVersionHeader(GaxPreconditions.CheckNotNull(spannerSettings, nameof(spannerSettings)));
SpannerSettings = AppendAssemblyVersionHeader(GaxPreconditions.CheckNotNull(spannerSettings, nameof(spannerSettings)));
Logger = GaxPreconditions.CheckNotNull(logger, nameof(logger));
_clientFactory = GaxPreconditions.CheckNotNull(clientFactory, nameof(clientFactory));
}
Expand All @@ -111,18 +110,16 @@ private static SpannerSettings AppendVersionHeader(SpannerSettings settings)
/// <param name="logger">The logger to use. May be null, in which case the default logger is used.</param>
/// <returns>A <see cref="SessionPoolManager"/> with the given options.</returns>
public static SessionPoolManager Create(SessionPoolOptions options, Logger logger = null) =>
new SessionPoolManager(options, DefaultSpannerSettings(), logger ?? Logger.DefaultLogger, CreateClientAsync);
new SessionPoolManager(options, CreateDefaultSpannerSettings(), logger ?? Logger.DefaultLogger, CreateClientAsync);

/// <summary>
/// Creates a <see cref="SessionPoolManager"/> with the specified SpannerSettings and options.
/// </summary>
/// <param name="spannerSettings">The SpannerSettings to use. May be null, in which case the default settings are used.</param>
/// <param name="options">The options to use. Must not be null.</param>
/// <param name="logger">The logger to use. May be null, in which case the logger returned by spannerSettings.Logger will be used.
/// If that is also null, the default logger is used.</param>
/// <param name="spannerSettings">The SpannerSettings to use. Must not be null.</param>
/// <returns>A <see cref="SessionPoolManager"/> with the given options.</returns>
public static SessionPoolManager Create(SessionPoolOptions options, SpannerSettings spannerSettings, Logger logger = null) =>
new SessionPoolManager(options, spannerSettings ?? DefaultSpannerSettings(), logger ?? spannerSettings?.Logger ?? Logger.DefaultLogger, CreateClientAsync);
public static SessionPoolManager CreateWithSettings(SessionPoolOptions options, SpannerSettings spannerSettings) =>
new SessionPoolManager(options, GaxPreconditions.CheckNotNull(spannerSettings, nameof(spannerSettings)).Clone(), spannerSettings.Logger ?? Logger.DefaultLogger, CreateClientAsync);

internal Task<SessionPool> AcquireSessionPoolAsync(SpannerClientCreationOptions options)
{
Expand Down

0 comments on commit d26b04c

Please sign in to comment.