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.

Sign up for GitHub

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

Jump to bottom

feat: custom endpoint plugin #369

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

feat: custom endpoint plugin #369

wants to merge 1 commit into from
+2,014 −203

Conversation

crystall-bitquill
Copy link
Contributor

@crystall-bitquill crystall-bitquill commented Jan 14, 2025 •
edited
Loading

Summary

Custom Endpoint Plugin

Description

  • Adds the custom endpoint plugin
  • Implements setAvailability in PluginService
  • Moves getRegion in the IamAuthUtils to RegionUtils
  • Fixes the MySQL database dialect getHostRole method - previously the query result was not being parsed correctly
  • Adds method to set the reader failover handler enableFailoverStrictReader value - previously it was always false.
  • Fixes incorrect check in the reader failover handler where a writer host would be returned even if the failover mode was set to strict reader
  • Moved the rdsHostListProvider's isDialectTopologyAware method to the utils file
  • Adds a check in the MySQL and PG Clients after creating a connection to confirm the host role, which can't always be identified correctly with just the provided host.

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

@crystall-bitquill crystall-bitquill added the ready for review Pull requests that are ready to be reviewed label Jan 14, 2025
karenc-bq
karenc-bq reviewed Jan 15, 2025
View reviewed changes
tests/integration/container/tests/custom_endpoint.test.ts
afterEach(async () => {
if (client !== null) {
try {
await client.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to clear the cache each test run via PluginManager.releaseResources()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, releaseResources is called below this line

@crystall-bitquill crystall-bitquill force-pushed the feat/custom-endpoints branch 7 times, most recently from 33a53d9 to ce12cb4 Compare January 24, 2025 00:57
sophia-bq
sophia-bq reviewed Jan 24, 2025
View reviewed changes
tests/integration/container/tests/read_write_splitting.test.ts Outdated Show resolved Hide resolved
tests/integration/container/tests/utils/aurora_test_utility.ts Outdated Show resolved Hide resolved
tests/integration/container/tests/utils/aurora_test_utility.ts Outdated Show resolved Hide resolved
tests/integration/container/tests/utils/aurora_test_utility.ts Outdated Show resolved Hide resolved
tests/unit/reader_failover_handler.test.ts Outdated Show resolved Hide resolved
docs/using-the-nodejs-wrapper/using-plugins/UsingTheCustomEndpointPlugin.md Outdated Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_plugin.ts Outdated Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_plugin.ts Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_plugin.ts Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_monitor_impl.ts Show resolved Hide resolved
@crystall-bitquill crystall-bitquill requested a review from a team as a code owner January 27, 2025 23:37
karenc-bq
karenc-bq reviewed Jan 28, 2025
View reviewed changes
common/lib/host_list_provider/rds_host_list_provider.ts Outdated Show resolved Hide resolved
common/lib/plugin_service.ts Outdated Show resolved Hide resolved
common/lib/plugin_service.ts Outdated Show resolved Hide resolved
common/lib/plugin_service.ts Outdated Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_monitor_impl.ts Outdated Show resolved Hide resolved
common/lib/plugins/custom_endpoint/custom_endpoint_monitor_impl.ts Outdated Show resolved Hide resolved
common/lib/plugins/failover/failover_plugin.ts Outdated Show resolved Hide resolved
common/lib/plugins/failover/failover_plugin.ts Show resolved Hide resolved
@crystall-bitquill crystall-bitquill force-pushed the feat/custom-endpoints branch 4 times, most recently from 33d730d to 1b72add Compare January 30, 2025 21:25
karenc-bq
karenc-bq reviewed Jan 31, 2025
View reviewed changes
common/lib/plugins/failover/failover_plugin.ts Outdated
Comment on lines 453 to 456
this.throwFailoverSuccessError();
} catch (error: any) {
this.failoverWriterFailedCounter.inc();
if (!(error instanceof FailoverSuccessError)) {
this.failoverWriterFailedCounter.inc();
Copy link
Contributor

Choose a reason for hiding this comment

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

throwFailoverSuccessError might throw TransactionResolutionUnknownError, so its better to call this.failoverWriterSuccessCounter.inc(); in the catch block

@crystall-bitquill crystall-bitquill force-pushed the feat/custom-endpoints branch 8 times, most recently from 311d1a2 to e977dbd Compare February 4, 2025 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophia-bq sophia-bq sophia-bq left review comments

@karenc-bq karenc-bq karenc-bq approved these changes

Assignees
No one assigned
Labels
ready for review Pull requests that are ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crystall-bitquill @karenc-bq @sophia-bq