Conversation
karenc-bq
force-pushed
the
feat/gdb-failover
branch
3 times, most recently
from
cc4c6aa to
bcc243b
Compare
karenc-bq
force-pushed
the
refactor/monitor-service
branch
from
81930fe to
00d8176
Compare
karenc-bq
force-pushed
the
feat/gdb-failover
branch
2 times, most recently
from
5b7069a to
14c1bb3
Compare
| } | ||
|
|
||
| forceMonitoringRefresh(shouldVerifyWriter: boolean, timeoutMs: number): Promise<HostInfo[]> { | ||
| throw new AwsWrapperError("ConnectionStringHostListProvider does not support forceMonitoringRefresh."); |
There was a problem hiding this comment.
Can we place this in messages?
| expect(returnConn).toBe(mockInitialClientWrapper); | ||
| }); | ||
|
|
||
| it("test_get_verified_connection_cluster_inet_address_none", async () => { |
There was a problem hiding this comment.
Might be good to add some tests for the new functionality of stale dns helper?
| 1320000 | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Wondering about adding some tests for when the failover would not be successful? ie. out of region or no allowed hosts?
| } | ||
|
|
||
| // Check writer region to determine failover mode | ||
| const writerRegion = this.rdsHelper.getRdsRegion(writerCandidate.host); |
There was a problem hiding this comment.
Can this return null? Would we have issues if it does?
There was a problem hiding this comment.
equalsIgnoreCase has null checks and would return false if writerRegion is null
karenc-bq
force-pushed
the
feat/gdb-failover
branch
from
14c1bb3 to
f253acb
Compare
karenc-bq
force-pushed
the
feat/gdb-failover
branch
from
f253acb to
6167281
Compare
|
|
||
| const value = cache.get(key); | ||
| if (value === null || value === undefined) { | ||
| if (!value) { |
There was a problem hiding this comment.
Wondering about this change? Example: if the values in the cache were booleans then False would become null
|
|
||
| try { | ||
| const updatedHostList: HostInfo[] = await hostListProvider.forceMonitoringRefresh(shouldVerifyWriter, timeoutMs); | ||
| const updatedHostList: HostInfo[] = await (hostListProvider as any).forceMonitoringRefresh(shouldVerifyWriter, timeoutMs); |
There was a problem hiding this comment.
| const updatedHostList: HostInfo[] = await (hostListProvider as any).forceMonitoringRefresh(shouldVerifyWriter, timeoutMs); | |
| const updatedHostList: HostInfo[] = await (hostListProvider as DynamicHostListProvider).forceMonitoringRefresh(shouldVerifyWriter, timeoutMs); |
Why are we casting to any instead of DynamicHostListProvider?
| "GlobalDbFailoverPlugin.isHomeRegion": "Is home region: %s", | ||
| "GlobalDbFailoverPlugin.currentFailoverMode": "Current Global DB failover mode: %s", | ||
| "GlobalDbFailoverPlugin.failoverElapsed": "Global DB failover elapsed time: %s ms", | ||
| "GlobalDbFailoverPlugin.candidateNull": "Candidate host is null for role: %s", |
There was a problem hiding this comment.
I'm wondering if we could find a better description and name for this error. It's not really capturing the cause which is that we couldn't find a host given the strategy and allowed host list.
| if (!(ex instanceof FailoverFailedError)) { | ||
| this.failoverWriterFailedCounter.inc(); |
There was a problem hiding this comment.
This is a bit confusing could we maybe add a comment explaining that we only want to increment the counter if we haven't already marked it and then thrown a failover failed error? Also what is the benefit to marking it above instead of just always incrementing it here? Is it about timing?
3 participants
Summary
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.