Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement configuration profiles #436

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karenc-bq
Copy link
Contributor

Description

Implement

  • connection profiles
  • initial connection strategy plugin
  • fix is_dialect bug returning False for different mysql connector libraries

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@karenc-bq karenc-bq force-pushed the configuration_profiles branch 4 times, most recently from c4880b1 to 91ff230 Compare February 28, 2024 19:15
@karenc-bq karenc-bq added wip Pull requests that are a work in progress ready for review Pull requests that are ready to be reviewed and removed wip Pull requests that are a work in progress labels Feb 28, 2024
@karenc-bq karenc-bq force-pushed the configuration_profiles branch 4 times, most recently from c721c6f to 6933c4f Compare February 28, 2024 21:44
def init_host_provider(self, props: Properties, host_list_provider_service: HostListProviderService, init_host_provider_func: Callable):
self._host_list_provider_service = host_list_provider_service
if host_list_provider_service.is_static_host_list_provider():
raise AwsWrapperError(Messages.get("AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be also log the error with the logger here?

@karenc-bq karenc-bq force-pushed the configuration_profiles branch from 6933c4f to 46e2a54 Compare March 19, 2024 22:13
"random")

OPEN_CONNECTION_RETRY_TIMEOUT_MS = WrapperProperty("open_connection_retry_timeout_ms",
"Maximum allowed time for the retries opening a connection.", 30_000)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum allowed time in milliseconds for the retries opening a connection.

@karenc-bq karenc-bq force-pushed the configuration_profiles branch from 46e2a54 to ad3d3d1 Compare March 20, 2024 23:10
@karenc-bq karenc-bq force-pushed the configuration_profiles branch from e5816eb to 0cf3aaf Compare April 26, 2024 06:56
@@ -25,6 +25,9 @@ AdfsCredentialsProviderFactory.SignOnPagePostActionRequestFailed=[AdfsCredential
AdfsCredentialsProviderFactory.SignOnPageRequestFailed=[AdfsCredentialsProviderFactory] ADFS SignOn Page Request Failed with HTTP status '{}', reason phrase '{}', and response '{}'
AdfsCredentialsProviderFactory.SignOnPageUrl=[AdfsCredentialsProviderFactory] ADFS SignOn URL: '{}'

AuroraInitialConnectionStrategyPlugin.UnsupportedStrategy=Unsupported host selection strategy '{}'.
Copy link
Collaborator

@joyc-bq joyc-bq May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AuroraInitialConnectionStrategyPlugin.UnsupportedStrategy=Unsupported host selection strategy '{}'.
AuroraInitialConnectionStrategyPlugin.UnsupportedStrategy=[AuroraInitialConnectionStrategyPlugin] Unsupported host selector strategy '{}'.

@@ -25,6 +25,9 @@ AdfsCredentialsProviderFactory.SignOnPagePostActionRequestFailed=[AdfsCredential
AdfsCredentialsProviderFactory.SignOnPageRequestFailed=[AdfsCredentialsProviderFactory] ADFS SignOn Page Request Failed with HTTP status '{}', reason phrase '{}', and response '{}'
AdfsCredentialsProviderFactory.SignOnPageUrl=[AdfsCredentialsProviderFactory] ADFS SignOn URL: '{}'

AuroraInitialConnectionStrategyPlugin.UnsupportedStrategy=Unsupported host selection strategy '{}'.
AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider=Dynamic host list provider is required.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider=Dynamic host list provider is required.
AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider=[AuroraInitialConnectionStrategyPlugin] Dynamic host list provider is required.


OPEN_CONNECTION_RETRY_TIMEOUT_MS = WrapperProperty("open_connection_retry_timeout_ms",
"Maximum allowed time in milliseconds for the retries opening a connection.", 30_000)
OPEN_CONNECTION_RETRY_INTERVAL_MS = WrapperProperty("open_connection_retry_interval_ms", "Time between each retry of opening a connection.", 1000)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OPEN_CONNECTION_RETRY_INTERVAL_MS = WrapperProperty("open_connection_retry_interval_ms", "Time between each retry of opening a connection.", 1000)
OPEN_CONNECTION_RETRY_INTERVAL_MS = WrapperProperty("open_connection_retry_interval_ms", "Time in milliseconds between each retry of opening a connection.", 1000)

Comment on lines +56 to +58
msg = Messages.get("AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider")
logger.warning(msg)
raise AwsWrapperError(msg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
msg = Messages.get("AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider")
logger.warning(msg)
raise AwsWrapperError(msg)
msg = "AuroraInitialConnectionStrategyPlugin.RequireDynamicProvider"
logger.warning(msg)
raise AwsWrapperError(Messages.get(msg))

Copy link
Collaborator

@joyc-bq joyc-bq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved with a few minor comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull requests that are ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants