feat: connection provider and connection provider manager #41
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
added
the
ready for review
joyc-bq
reviewed
May 2, 2024
| const availableHost = new HostInfoBuilder({ | ||
| hostAvailabilityStrategy: new SimpleHostAvailabilityStrategy() | ||
| }) | ||
| .withHost("someAvailableHost") |
There was a problem hiding this comment.
nit: "someAvailableHost"/"someUnavailableHost" could be const variable
joyc-bq
approved these changes
May 2, 2024
karenc-bq
reviewed
May 3, 2024
Comment on lines
35
to
37
| if (ConnectionProviderManager.connProvider) { | ||
| if (ConnectionProviderManager.connProvider.acceptsUrl(hostInfo, props)) { | ||
| return ConnectionProviderManager.connProvider; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (ConnectionProviderManager.connProvider) { | |
| if (ConnectionProviderManager.connProvider.acceptsUrl(hostInfo, props)) { | |
| return ConnectionProviderManager.connProvider; | |
| } | |
| } | |
| if (ConnectionProviderManager.connProvider?.acceptsUrl(hostInfo, props)) { | |
| return ConnectionProviderManager.connProvider; | |
| } |
karenc-bq
reviewed
May 3, 2024
| acceptsStrategy(role: HostRole, strategy: string) { | ||
| let acceptsStrategy = false; | ||
| if (ConnectionProviderManager.connProvider) { | ||
| acceptsStrategy = ConnectionProviderManager.connProvider.acceptsStrategy(role, strategy); | ||
| } | ||
|
|
||
| if (!acceptsStrategy) { | ||
| acceptsStrategy = this.defaultProvider.acceptsStrategy(role, strategy); | ||
| } | ||
|
|
||
| return acceptsStrategy; | ||
| } |
There was a problem hiding this comment.
What do you think of this?
Suggested change
| acceptsStrategy(role: HostRole, strategy: string) { | |
| let acceptsStrategy = false; | |
| if (ConnectionProviderManager.connProvider) { | |
| acceptsStrategy = ConnectionProviderManager.connProvider.acceptsStrategy(role, strategy); | |
| } | |
| if (!acceptsStrategy) { | |
| acceptsStrategy = this.defaultProvider.acceptsStrategy(role, strategy); | |
| } | |
| return acceptsStrategy; | |
| } | |
| acceptsStrategy(role: HostRole, strategy: string) { | |
| return ConnectionProviderManager.connProvider?.acceptsStrategy(role, strategy) || this.defaultProvider.acceptsStrategy(role, strategy); | |
| } |
karenc-bq
reviewed
May 3, 2024
|
|
||
| getHostInfoByStrategy(hosts: HostInfo[], role: HostRole, strategy: string, props: Map<string, any>) { | ||
| let host; | ||
| if (ConnectionProviderManager.connProvider && ConnectionProviderManager.connProvider.acceptsStrategy(role, strategy)) { |
There was a problem hiding this comment.
Are these equivalent?
if (ConnectionProviderManager.connProvider && ConnectionProviderManager.connProvider.acceptsStrategy(role, strategy)) {
and
if (ConnectionProviderManager.connProvider?.acceptsStrategy(role, strategy)) {
87c3b95 to
e78fce6
Compare
karenc-bq
reviewed
May 6, 2024
common/lib/plugins/default_plugin.ts
Outdated
| @@ -57,7 +80,30 @@ export class DefaultPlugin extends AbstractConnectionPlugin { | |||
| connectFunc: () => Promise<Type> | |||
| ): Promise<Type> { | |||
| logger.debug(`Start connect for test plugin: ${this.id}`); | |||
There was a problem hiding this comment.
Could you please remove this log?
karenc-bq
reviewed
May 6, 2024
common/lib/random_host_selector.ts
Outdated
| getHost(hosts: HostInfo[], role: HostRole, props?: Map<string, any>): HostInfo { | ||
| const eligibleHosts = hosts.filter((hostInfo: HostInfo) => hostInfo.role === role && hostInfo.getAvailability() === HostAvailability.AVAILABLE); | ||
| if (eligibleHosts.length === 0) { | ||
| throw new AwsWrapperError(Messages.get("HostSelector.noHostsMatchingRole", role.toString())); |
There was a problem hiding this comment.
This will print No hosts were found matching the requested '1' role. Need to update the HostRole class to
export enum HostRole {
UNKNOWN = "unknown",
WRITER = "writer",
READER = "reader"
}then you can just
Suggested change
| throw new AwsWrapperError(Messages.get("HostSelector.noHostsMatchingRole", role.toString())); | |
| throw new AwsWrapperError(Messages.get("HostSelector.noHostsMatchingRole", role)); |
karenc-bq
approved these changes
May 6, 2024
crystall-bitquill
force-pushed
the
connection-provider
branch
from
e78fce6 to
9937cb2
Compare
joyc-bq
approved these changes
May 7, 2024
joyc-bq
reviewed
May 7, 2024
| props: Map<string, any>, | ||
| defaultConnProvider: ConnectionProvider, | ||
| effectiveConnProvider: ConnectionProvider | null | ||
| ): ConnectionPlugin[] { | ||
| const plugins: ConnectionPlugin[] = []; | ||
| let pluginCodes: string = props.get(WrapperProperties.PLUGINS.name); | ||
| if (pluginCodes === null || pluginCodes === undefined) { |
There was a problem hiding this comment.
could this be simplified to pluginCodes == null? Or do you think it is better practice to be explicit as you've done here?
joyc-bq
reviewed
May 7, 2024
common/lib/utils/locales/en.json
Outdated
| @@ -1,9 +1,11 @@ | |||
| { | |||
| "ConnectionPluginManager.unknownPluginCode": "Unknown plugin code: '%s'", | |||
| "ConnectionPluginManager.failedToConnectWithNewTargetClient": "Failed to connect with a new target for host: '%s'", | |||
| "ConnectionProvider.unsupportedHostSpecSelectorStrategy": "Unsupported host selection strategy '%s' specified for this connection provider '%s'. Please visit the documentation for all supported strategies.", | |||
There was a problem hiding this comment.
Suggested change
| "ConnectionProvider.unsupportedHostSpecSelectorStrategy": "Unsupported host selection strategy '%s' specified for this connection provider '%s'. Please visit the documentation for all supported strategies.", | |
| "ConnectionProvider.unsupportedHostInfoSelectorStrategy": "Unsupported host selection strategy '%s' specified for this connection provider '%s'. Please visit the documentation for all supported strategies.", |
|
lgtm just a couple small comments |
crystall-bitquill
force-pushed
the
connection-provider
branch
from
9937cb2 to
dfb0a93
Compare
leszekamz
reviewed
May 8, 2024
| const container = new PluginServiceManagerContainer(); | ||
| this.pluginService = new PluginService(container, this); | ||
| this.pluginManager = new PluginManager(container, this.properties); | ||
| this.pluginService = new PluginService(container, this, this._properties); |
There was a problem hiding this comment.
Nit:
this._properties - the member is used directly.
However, the call below uses this.properties (which is a getter for this._properties)
The result is the same, but should we choose the same in both cases, either properties or _properties?
crystall-bitquill
force-pushed
the
connection-provider
branch
from
dfb0a93 to
b374878
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.