Conversation
| }); | ||
|
|
||
| if (EnabledFileLog) | ||
| if (true) |
There was a problem hiding this comment.
Reminder to revert this change before merge unless this is intentional
| { | ||
| return this.pluginManager.IsPluginActive(pluginName); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Curious, could this.pluginManager.IsPluginActive(pluginName) actually throw errors?
|
|
||
| protected static readonly string AuroraPostgreSqlBgStatusQuery = | ||
| "SELECT * FROM " + | ||
| $"pg_catalog.get_blue_green_fast_switchover_metadata('aws_dotnet_driver')"; |
There was a problem hiding this comment.
nit: could we use the full wrapper name instead of aws_dotnet_driver?
|
|
||
| public static bool IsBlueGreenConnectionDialect(IDialect dialect) | ||
| { | ||
| return dialect is AuroraMySqlDialect or AuroraPgDialect or RdsMySqlDialect or RdsPgDialect; |
There was a problem hiding this comment.
Say customer is using RdsMySQLDialect. Will this code error out on dialect is AuroraMySqlDialect because AuroraMySqlDialect is not registered?
| try | ||
| { | ||
| using var cmd = conn.CreateCommand(); | ||
| cmd.CommandText = existenceQuery; |
There was a problem hiding this comment.
Do we need to set a command timeout for all these internal queries?
|
I think the csproj file is missing for BlueGreenConnection and BlueGreenConnection.Tests projects. |
|
|
||
| namespace AwsWrapperDataProvider.Plugin.BlueGreenConnection.BlueGreenConnection; | ||
|
|
||
| public class BlueGreenConnectionPluginService : PluginService |
There was a problem hiding this comment.
when is this BlueGreenConnectionPluginService used?
There was a problem hiding this comment.
+1, I don't see it being used so we can probably remove it.
|
|
||
| protected int GetValueHash(int currentHash, string val) | ||
| { | ||
| return currentHash * 31 + val.GetHashCode(); |
There was a problem hiding this comment.
why do we multiply by 31 + val.GetHashCode()? I see jdbc also does this but it seem quite arbitrary
|
|
||
| if (!BlueGreenStatusMapping.TryGetValue(value.ToUpperInvariant(), out var phase)) | ||
| { | ||
| throw new ArgumentException($"Unknown blue/green deployment status: {value}"); |
There was a problem hiding this comment.
do we need to add to resx file
|
|
||
| Logger.LogTrace(Resources.SuspendConnectRouting_Apply_SwitchoverCompleteContinueWithConnect, (this.GetNanoTime() - holdStartTime) / 1_000_000); | ||
| } | ||
| catch (OperationCanceledException) |
There was a problem hiding this comment.
Is an OperationCanceledException ever thrown by Delay? base delay seems to just return when cancellationToken.IsCancellationRequested.
This might be an issue as we might be returning null when cts is cancelled, which assumes the apply was successful.
| return DateTime.UtcNow.Ticks * 100; | ||
| } | ||
|
|
||
| protected void Delay(long delayMs, BlueGreenStatus? bgStatus, string bgdId, CancellationToken cancellationToken) |
There was a problem hiding this comment.
It might be worth turning Delay to an async function too and use Task.Await instead of Thread.Sleep, the reason being is that Task.Await will free up the thread pool and allow other thread workers to use the thread, while Thread.Sleep keeps hold of the thread.
|
|
||
| Logger.LogTrace(Resources.SuspendExecuteRouting_Apply_SwitchoverCompletedContinueWithMethod, methodName, (this.GetNanoTime() - holdStartTime) / 1_000_000); | ||
| } | ||
| catch (OperationCanceledException) |
kenrickyap
left a comment
kenrickyap
left a comment
There was a problem hiding this comment.
NOICE WORK :):) added some comments
| protected static readonly string RdsPgBgStatusQuery = | ||
| $"SELECT * FROM rds_tools.show_topology('aws_dotnet_driver')"; | ||
|
|
||
|
|
| "SELECT * FROM " + | ||
| $"pg_catalog.get_blue_green_fast_switchover_metadata('aws_dotnet_driver')"; | ||
|
|
||
| protected static readonly string RdsMySqlTopologyTableExistsQuery = |
There was a problem hiding this comment.
Aurora and RDS MySQL have the same queries. Maybe we can just use the same variables here.
|
|
||
| namespace AwsWrapperDataProvider.Plugin.BlueGreenConnection.BlueGreenConnection; | ||
|
|
||
| public delegate void OnBlueGreenStatusChange(BlueGreenRoleType role, BlueGreenInterimStatus interimStatus); |
There was a problem hiding this comment.
Curious if we could just move this to BlueGreenStatusMonitor since it is only used there.
|
|
||
| private int monitorResetOnInProgressCompleted; // 0 = false, 1 = true | ||
| private int monitorResetOnTopologyCompleted; | ||
|
|
| : long.Parse(PropertyDefinition.BgIntervalIncreasedMs.DefaultValue!); | ||
| this.statusCheckIntervalMap[BlueGreenIntervalRate.HIGH] = | ||
| !props.TryGetValue(PropertyDefinition.BgIntervalHighMs.Name, out var high) | ||
| ? long.Parse(PropertyDefinition.BgIntervalHighMs.DefaultValue!) |
There was a problem hiding this comment.
nit: theres a blank space at the end of this line
| List<IConnectRouting> connectRouting, | ||
| List<IExecuteRouting> executeRouting, | ||
| IDictionary<string, BlueGreenRoleType> roleByHost, | ||
| IDictionary<string, (HostSpec, HostSpec)> correspondingNodes) |
There was a problem hiding this comment.
IDictionary<string, (HostSpec, HostSpec?)>
...rapperDataProvider.Plugin.BlueGreenConnection/BlueGreenConnection/BlueGreenStatusProvider.cs
Show resolved
Hide resolved
| if (!this.blueDnsUpdateCompleted || Interlocked.CompareExchange(ref this.allGreenNodesChangedName, 0, 0) == 0) | ||
| { | ||
| // New connect calls to blue nodes should be routed to green nodes. | ||
| foreach (var x in this.roleByHost.Where(x => x.Value == BlueGreenRoleType.SOURCE && this.correspondingNodes.ContainsKey(x.Key))) |
There was a problem hiding this comment.
Curious if you could just do
var (blueHost, value)instead of var x here for easier processing later on
|
|
||
| // Check whether green host is already been connected with blue (no-prefixes) IAM host name. | ||
| List<HostSpec> iamHosts; | ||
| if (this.IsAlreadySuccessfullyConnected(greenHost, blueHost)) |
There was a problem hiding this comment.
HostSpec iamBlueHost = this.hostSpecBuilder.CopyFrom(greenHostSpec).WithHost(BlueGreenConnectionUtils.RemoveGreenInstancePrefix(greenHost)).Build();
if (this.IsAlreadySuccessfullyConnected(greenHost, iamBlueHost.Host))|
|
||
|
|
| } | ||
|
|
||
| public override async Task<DbConnection> ForceOpenConnection( | ||
| HostSpec hostSpec, |
| } | ||
|
|
||
| public override async Task<DbConnection> OpenConnection( | ||
| HostSpec hostSpec, |
| : await pluginService.OpenConnection(this.SubstituteHostSpec, props, plugin, false); | ||
| } | ||
|
|
||
| bool iamInUse = pluginService.IsPluginInUse("IamAuthPlugin"); |
There was a problem hiding this comment.
We should be having constants instead of hard-coded strings.
Also I don't think we should be using the class name, it would be better to do the plugin code.
|
|
||
| public bool IsPluginActive(string pluginName) | ||
| { | ||
| return this.plugins.Any(p => p.GetType().Name.Contains(pluginName, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
I'm not the biggest fan of using the class name as a string.
Perhaps something like this would be better?
public bool IsPluginActive<T>() where T : IConnectionPlugin
{
return this.plugins.Any(p => p is T);
}
bool iamInUse = pluginService.IsPluginInUse<IamAuthPlugin>();
This would require you to have a dependency tho with IamAuthPlugin. But if you can't import the class like Java, then we should do something like GO and use the plugin code instead. It's better cause that would remain constant to what users use.
public bool IsPluginActive(string pluginCode)
{
return this.activePluginCodes.Contains(pluginCode);
}
where plugin codes is the codes used. You can make this an array, or a hash set to be more effecient. You would probably need to make this when building the plugin chain and parsing the dns.
There was a problem hiding this comment.
Ya I agree. This was only done because I wanted to make a seperate package but that doesnt seem needed.
| protected static readonly string AuroraMySqlBgTopologyExistsQuery = | ||
| "SELECT 1 AS tmp FROM information_schema.tables WHERE" + | ||
| " table_schema = 'mysql' AND table_name = 'rds_topology'"; |
There was a problem hiding this comment.
Curious, what's the purpose of making these into helpers and not being in the dialect classes?
Also, curious why BG is another project? For visibilty, main purpose of the other drivers separating the plugins is to avoid having SDK dependencies if they don't need it. I don' tthink blue green has these dependencies.
| AuroraMySqlDialect => CheckExistenceQueries(connection, AuroraMySqlBgTopologyExistsQuery), | ||
| AuroraPgDialect => CheckExistenceQueries(connection, AuroraPostgreSqlBgTopologyExistsQuery), | ||
| RdsMySqlDialect => CheckExistenceQueries(connection, RdsMySqlTopologyTableExistsQuery), | ||
| RdsPgDialect => CheckExistenceQueries(connection, RdsPgTopologyTableExistsQuery), |
There was a problem hiding this comment.
This isn't very OOP-like imo and goes against it, we should be depending on their underlying implementation and use the dialect class.
There was a problem hiding this comment.
Will be fixed when it is moved to one package
| // Network-bound methods that might fail and trigger failover | ||
| "DbConnection.Open", | ||
| "DbConnection.OpenAsync", | ||
| "DbConnection.BeginDbTransaction", | ||
| "DbConnection.BeginDbTransactionAsync", |
There was a problem hiding this comment.
We should really keep a list of these in the driver dialect or something to make it all consistent
...pperDataProvider.Plugin.BlueGreenConnection/BlueGreenConnection/BlueGreenConnectionPlugin.cs
Show resolved
Hide resolved
| this.pluginService = pluginService; | ||
| this.props = props; | ||
| this.providerSupplier = providerSupplier; | ||
| this.bgdId = PropertyDefinition.BgdId.GetString(this.props); |
There was a problem hiding this comment.
Should we normalize this like jdbc? Require it to be non null, trim, and also lowercase?
And curious, can users ever pass in null through the properties? This may blow up and we will get a NPE if we pass this in to routing.apply. Perhaps we should make bgId string instead of string? if possible
...pperDataProvider.Plugin.BlueGreenConnection/BlueGreenConnection/BlueGreenConnectionPlugin.cs
Show resolved
Hide resolved
|
|
||
| protected virtual long GetNanoTime() | ||
| { | ||
| return DateTime.UtcNow.Ticks * 100; |
There was a problem hiding this comment.
Is this the right function or unit? Ticks are already measured in 100 nanoseconds. Multiplying this by 100 would not be correct.
If we want to be more precise we should use Stopwatch.GetTimestamp() instead.
| public static readonly AwsWrapperProperty BgConnectTimeout = new( | ||
| "bgConnectTimeoutMs", | ||
| "30000", | ||
| "Connect timeout (in msec) during Blue/Green Deployment switchover."); |
There was a problem hiding this comment.
I also see this in jdbc, but it doesn't look like this is configurable to the user?
| protected readonly HostSpec initialHostSpec; | ||
|
|
||
| private readonly CancellationTokenSource concellationTokenSource = new(); | ||
| private readonly SemaphoreSlim sleepWaitObj = new(0); |
There was a problem hiding this comment.
Is using a semaphore the correct approach? These are often used for counts. If we have multiple NotifyAlls and it accumulates, then the subsequent delays will be omitted.
| } | ||
| }); | ||
|
|
||
| await this.openConnectionTask; |
There was a problem hiding this comment.
fyi, this will wait for the task to finish. At that point, is there a reason to make it async? JDBC does completely in a different thread in parallel. If you want, we can make it simpler and just make this process synchronous and just remove the Task.run().
| this.props[PropertyDefinition.Host.Name] = connectionHostSpecCopy.Host; | ||
| this.props[PropertyDefinition.Port.Name] = connectionHostSpecCopy.Port.ToString(); | ||
| this.hostListProvider = this.pluginService.Dialect.HostListProviderSupplier(this.props, (PluginService)this.pluginService, this.pluginService); | ||
| } |
There was a problem hiding this comment.
this is mutating the actual props, did you mean to use the copy?
| this.role, | ||
| hostListProperties["ClusterId"]); | ||
|
|
||
| var connectionHostSpecCopy = this.connectionHostSpec; |
There was a problem hiding this comment.
Is this a copy? Or are we just setting the reference to the same object? We need this to be a full copy.
| protected readonly Dictionary<BlueGreenIntervalRate, long> statusCheckIntervalMap; | ||
| protected readonly HostSpec initialHostSpec; | ||
|
|
||
| private readonly CancellationTokenSource concellationTokenSource = new(); |
There was a problem hiding this comment.
typo: should be cancellationTokenSource
...rapperDataProvider.Plugin.BlueGreenConnection/BlueGreenConnection/BlueGreenStatusProvider.cs
Show resolved
Hide resolved
...rapperDataProvider.Plugin.BlueGreenConnection/BlueGreenConnection/BlueGreenStatusProvider.cs
Show resolved
Hide resolved
|
|
||
| protected long GetNanoTime() | ||
| { | ||
| return DateTime.UtcNow.Ticks * 100; |
There was a problem hiding this comment.
Same comment with ticks here.
| + divider | ||
| + string.Join("\n", this.phaseTimeNano | ||
| .OrderBy(y => y.Value.TimestampNano) | ||
| .Select(x => string.Format("{0,28} {1,18} ms {{2," + maxEventNameLength + "}}", |
There was a problem hiding this comment.
Is the double-braces in {{2," + , intentional?
4 participants
Summary
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.