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
from
bcc243b to
5b7069a
Compare
karenc-bq
force-pushed
the
feat/gdb-failover
branch
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?
| } | ||
|
|
||
| override async failoverReader(): Promise<void> { | ||
| throw new UnsupportedMethodError("This method should not be used in this class. See failover() method for implementation details."); |
There was a problem hiding this comment.
Can we add these to the messages file?
| 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
| return new Promise((resolve) => setTimeout(resolve, 100)); | ||
| } | ||
|
|
||
| override async failoverReader(): Promise<void> { |
There was a problem hiding this comment.
Just to throw a thought out there, would it make sense to just make the failoverReader method private in the base class and not override these to do nothing?
Answer to that question might be more on what other failover plugins we plan to do.
| } | ||
|
|
||
| logger.debug(Messages.get("Failover.establishedConnection", this.pluginService.getCurrentHostInfo()?.host ?? "unknown")); | ||
| this.throwFailoverSuccessException(); |
There was a problem hiding this comment.
In terms of telemetry, I think we only record it in failoverToWriter. But I don't think we do it for failoverToAllowedHost right?
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.