feat: failover plugin #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crystall-bitquill
force-pushed
the
failover-plugin
branch
3 times, most recently
from
71f08bc to
61373ca
Compare
crystall-bitquill
added
the
ready for review
sergiyvamz
reviewed
Apr 5, 2024
| @@ -19,6 +19,9 @@ import { HostListProvider } from "./host_list_provider/host_list_provider"; | |||
| import { HostListProviderService } from "./host_list_provider_service"; | |||
|
|
|||
| export interface DatabaseDialect { | |||
| getConnectFunc(newTargetClient: AwsClient): () => Promise<any>; | |||
There was a problem hiding this comment.
getConnectFunc is good for Java where there's no promises. For Node.js/TypeScript is quite natural when a method name has no prefixes like Func. We might consider review variable and method names to make them look more TypeScript natural. It could be a separate PR for that.
sergiyvamz
approved these changes
Apr 5, 2024
joyc-bq
approved these changes
Apr 8, 2024
karenc-bq
reviewed
Apr 10, 2024
|
|
||
| protected async internalPostConnect() { | ||
| const info = this.pluginService.getCurrentHostInfo(); | ||
| if (info != null) { |
There was a problem hiding this comment.
Does it make sense to move this.isConnected = true in the connect methods to internalPostConnect? Since it is only ever called after a successful connect.
karenc-bq
reviewed
Apr 10, 2024
|
|
||
| this.hostList = results.hosts; | ||
| return Array.from(this.hostList); | ||
| const currentClient = targetClient ? targetClient : this.hostListProviderService.getCurrentClient().targetClient; |
There was a problem hiding this comment.
Suggested change
| const currentClient = targetClient ? targetClient : this.hostListProviderService.getCurrentClient().targetClient; | |
| const currentClient = targetClient ?? this.hostListProviderService.getCurrentClient().targetClient; |
karenc-bq
reviewed
Apr 10, 2024
| const currentClient = client ? client : this.hostListProviderService.getCurrentClient(); | ||
| const results: FetchTopologyResult = await this.getTopology(currentClient, false); | ||
| logger.debug(logTopology(results.hosts, results.isCachedData ? "[From cache]" : "")); | ||
| const currentClient = targetClient ? targetClient : this.hostListProviderService.getCurrentClient().targetClient; |
There was a problem hiding this comment.
Suggested change
| const currentClient = targetClient ? targetClient : this.hostListProviderService.getCurrentClient().targetClient; | |
| const currentClient = targetClient ??: this.hostListProviderService.getCurrentClient().targetClient; |
karenc-bq
reviewed
Apr 10, 2024
Comment on lines
163
to
164
| this.hostList = results.hosts; | ||
| return Array.from(this.hostList); |
There was a problem hiding this comment.
Suggested change
| this.hostList = results.hosts; | |
| return Array.from(this.hostList); | |
| this.hostList = results.hosts; | |
| return this.hostList; |
karenc-bq
reviewed
Apr 10, 2024
common/lib/plugin_service.ts
Outdated
Comment on lines
245
to
249
| if (targetClient) { | ||
| await this.getDialect().tryClosingTargetClient(targetClient); | ||
| } else { | ||
| await this.getDialect().tryClosingTargetClient(this._currentClient.targetClient); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (targetClient) { | |
| await this.getDialect().tryClosingTargetClient(targetClient); | |
| } else { | |
| await this.getDialect().tryClosingTargetClient(this._currentClient.targetClient); | |
| } | |
| await this.getDialect().tryClosingTargetClient(targetClient ?? this._currentClient.targetClient); |
karenc-bq
reviewed
Apr 10, 2024
| ["reader-or-writer", FailoverMode.READER_OR_WRITER] | ||
| ]); | ||
|
|
||
| export function failoverModeFromValue(value: string): FailoverMode | null { |
There was a problem hiding this comment.
Should we add an "Unknown" FailoverMode and return that instead of null to show more intent?
karenc-bq
reviewed
Apr 10, 2024
Comment on lines
30
to
33
| if (!value || value === "") { | ||
| return null; | ||
| } | ||
| return nameToValue.get(value.toLowerCase()) ?? null; |
There was a problem hiding this comment.
Suggested change
| if (!value || value === "") { | |
| return null; | |
| } | |
| return nameToValue.get(value.toLowerCase()) ?? null; | |
| return nameToValue.get(value?.toLowerCase()) ?? null; |
karenc-bq
reviewed
Apr 10, 2024
Comment on lines
136
to
140
| if (this._rdsUrlType.isRdsCluster) { | ||
| this.failoverMode = this._rdsUrlType === RdsUrlType.RDS_READER_CLUSTER ? FailoverMode.READER_OR_WRITER : FailoverMode.STRICT_WRITER; | ||
| } else { | ||
| this.failoverMode = FailoverMode.STRICT_WRITER; | ||
| } |
There was a problem hiding this comment.
Do you need to check for isRdsCluster here if you are not doing anything with other cluster types?
Suggested change
| if (this._rdsUrlType.isRdsCluster) { | |
| this.failoverMode = this._rdsUrlType === RdsUrlType.RDS_READER_CLUSTER ? FailoverMode.READER_OR_WRITER : FailoverMode.STRICT_WRITER; | |
| } else { | |
| this.failoverMode = FailoverMode.STRICT_WRITER; | |
| } | |
| if (this.failoverMode == null) { | |
| this.failoverMode = this._rdsUrlType === RdsUrlType.RDS_READER_CLUSTER ? FailoverMode.READER_OR_WRITER : FailoverMode.STRICT_WRITER; | |
| } |
karenc-bq
reviewed
Apr 10, 2024
Comment on lines
265
to
267
| const res = methodFunc(); | ||
| logger.debug(`Execution time for plugin ${this.id}: ${performance.now() - start} ms`); | ||
| return res; |
There was a problem hiding this comment.
Suggested change
| const res = methodFunc(); | |
| logger.debug(`Execution time for plugin ${this.id}: ${performance.now() - start} ms`); | |
| return res; | |
| return methodFunc(); |
karenc-bq
reviewed
Apr 10, 2024
Comment on lines
279
to
281
| const res = await methodFunc(); | ||
| logger.debug(`Execution time for plugin ${this.id}: ${performance.now() - start} ms`); | ||
| return res; |
There was a problem hiding this comment.
Suggested change
| const res = await methodFunc(); | |
| logger.debug(`Execution time for plugin ${this.id}: ${performance.now() - start} ms`); | |
| return res; | |
| return await methodFunc(); |
crystall-bitquill
force-pushed
the
failover-plugin
branch
4 times, most recently
from
b45ac3d to
1fdfba4
Compare
karenc-bq
approved these changes
Apr 16, 2024
crystall-bitquill
force-pushed
the
failover-plugin
branch
from
1fdfba4 to
898999d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.