feat: session state transfer #51
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
session-state
branch
from
d585659 to
a6183c6
Compare
joyc-bq
reviewed
May 13, 2024
| @@ -106,7 +106,7 @@ export class DriverConnectionProvider implements ConnectionProvider { | |||
| getHostInfoByStrategy(hosts: HostInfo[], role: HostRole, strategy: string, props?: Map<string, any>): HostInfo { | |||
| const acceptedStrategy = DriverConnectionProvider.acceptedStrategies.get(strategy); | |||
| if (!acceptedStrategy) { | |||
| throw new AwsWrapperError(Messages.get("ConnectionProvider.unsupportedHostSpecSelectorStrategy", strategy, "DriverConnectionProvider")); // TODO | |||
| throw new AwsWrapperError(Messages.get("ConnectionProvider.unsupportedHostSpecSelectorStrategy", strategy, "DriverConnectionProvider")); | |||
There was a problem hiding this comment.
Suggested change
| throw new AwsWrapperError(Messages.get("ConnectionProvider.unsupportedHostSpecSelectorStrategy", strategy, "DriverConnectionProvider")); | |
| throw new AwsWrapperError(Messages.get("ConnectionProvider.unsupportedHostInfoSelectorStrategy", strategy, "DriverConnectionProvider")); |
joyc-bq
reviewed
May 13, 2024
common/lib/plugin_service.ts
Outdated
| const changes = new Set<HostChangeOptions>([HostChangeOptions.INITIAL_CONNECTION]); | ||
|
|
||
| if (this.pluginServiceManagerContainer.pluginManager) { | ||
| // this.pluginServiceManagerContainer.pluginManager.notifyConnectionChanged(changes, null); |
There was a problem hiding this comment.
maybe add comment:
// TODO: uncomment once notifyConnectionChanged is implemented.
joyc-bq
reviewed
May 13, 2024
common/lib/utils/locales/en.json
Outdated
| @@ -61,5 +61,7 @@ | |||
| "AuroraStaleDnsHelper.clusterEndpointDns": "Cluster endpoint resolves to '%s'.", | |||
| "AuroraStaleDnsHelper.writerHostSpec": "Writer host: '%s'.", | |||
| "AuroraStaleDnsHelper.writerInetAddress": "Writer host address: '%s'", | |||
| "AuroraStaleDnsHelper.staleDnsDetected": "Stale DNS data detected. Opening a connection to '%s'." | |||
| "AuroraStaleDnsHelper.staleDnsDetected": "Stale DNS data detected. Opening a connection to '%s'.", | |||
| "MethodNotSupported": "Method not supported.", | |||
There was a problem hiding this comment.
Suggested change
| "MethodNotSupported": "Method not supported.", | |
| "Client.MethodNotSupported": "Method not supported.", |
should we also specify which method is not supported since this is used for a few different methods
joyc-bq
reviewed
May 13, 2024
common/lib/utils/locales/en.json
Outdated
| "AuroraStaleDnsHelper.staleDnsDetected": "Stale DNS data detected. Opening a connection to '%s'." | ||
| "AuroraStaleDnsHelper.staleDnsDetected": "Stale DNS data detected. Opening a connection to '%s'.", | ||
| "MethodNotSupported": "Method not supported.", | ||
| "InvalidTransactionIsolationLevel": "An invalid transaction isolation level was provided." |
There was a problem hiding this comment.
Suggested change
| "InvalidTransactionIsolationLevel": "An invalid transaction isolation level was provided." | |
| "Client.InvalidTransactionIsolationLevel": "An invalid transaction isolation level was provided." |
nit: should we specify the isolation level provided in the error message?
joyc-bq
approved these changes
May 14, 2024
crystall-bitquill
force-pushed
the
session-state
branch
from
a6183c6 to
c12c2cd
Compare
karenc-bq
reviewed
May 15, 2024
| this.sessionState.autoCommit.pristineValue = this.pluginService.getCurrentClient().getAutoCommit(); | ||
| } | ||
|
|
||
| this.logCurrentState(); |
There was a problem hiding this comment.
Does it make sense to log the current states when updating pristine values? The log message doesn't actually output any information about the pristine states.
karenc-bq
reviewed
May 15, 2024
| try { | ||
| await newClient.setReadOnly(this.sessionState.readOnly.value); | ||
| } catch (error: any) { | ||
| if (error instanceof Error && error.message.includes("Method not supported")) { |
There was a problem hiding this comment.
Is this condition checking a custom error that we implemented? If so can we catch for a more specific class instead of the generic Error?
export class UnsupportedMethod extends AwsWrapperError {Then
if (error instanceof UnsupportedMethod)
crystall-bitquill
force-pushed
the
session-state
branch
4 times, most recently
from
f09095c to
079d581
Compare
joyc-bq
reviewed
May 23, 2024
common/lib/utils/locales/en.json
Outdated
Comment on lines
69
to
70
| "Client.MethodNotSupported": "Method not supported.", | ||
| "Client.InvalidTransactionIsolationLevel": "An invalid transaction isolation level was provided: '%s'." |
There was a problem hiding this comment.
Suggested change
| "Client.MethodNotSupported": "Method not supported.", | |
| "Client.InvalidTransactionIsolationLevel": "An invalid transaction isolation level was provided: '%s'." | |
| "Client.methodNotSupported": "Method not supported.", | |
| "Client.invalidTransactionIsolationLevel": "An invalid transaction isolation level was provided: '%s'." |
There was a problem hiding this comment.
will also need to update where these messages are used
crystall-bitquill
force-pushed
the
session-state
branch
from
079d581 to
a6e648e
Compare
karenc-bq
approved these changes
May 23, 2024
crystall-bitquill
force-pushed
the
session-state
branch
from
a6e648e to
30e2816
Compare
jasonlamz
pushed a commit
that referenced
this pull request
May 27, 2024
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.