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: failover2 #363

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: failover2 #363

wants to merge 3 commits into from

Conversation

sophia-bq
Copy link
Contributor

Summary

Description

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

@sophia-bq sophia-bq added the wip Pull requests that are a work in progress label Jan 3, 2025
@sophia-bq sophia-bq requested a review from sergiyvamz January 3, 2025 18:07
@sophia-bq sophia-bq force-pushed the feat/failover2 branch 2 times, most recently from c4a4778 to 871729d Compare January 7, 2025 04:05
@sophia-bq sophia-bq force-pushed the feat/failover2 branch 3 times, most recently from 4a749c6 to f86dc16 Compare January 9, 2025 19:02
karenc-bq
karenc-bq reviewed Jan 13, 2025
View reviewed changes
common/lib/plugins/failover2/failover2_plugin.ts

// Signal to connect that this is an internal call and does not require additional processing.
const copyProps = new Map<string, any>(this._properties);
copyProps.set(Failover2Plugin.INTERNAL_CONNECT_PROPERTY_NAME, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This new property will cause the community MySQL driver to warn

Ignoring invalid configuration option passed to Connection: 76c06979-49c4-4c86-9600-a63605b83f50.

Please make sure it is handled by removeWrapperProperties

@sophia-bq sophia-bq mentioned this pull request Jan 20, 2025
Open
@sophia-bq sophia-bq removed the wip Pull requests that are a work in progress label Jan 23, 2025
joyc-bq
joyc-bq reviewed Jan 24, 2025
View reviewed changes
common/lib/host_list_provider/monitoring/cluster_topology_monitor.ts
this.monitor.hostMonitorsWriterClient = client;
this.monitor.hostMonitorsWriterInfo = this.hostInfo;
this.monitor.hostMonitorsStop = true;
this.monitor.logTopology(`[hostMonitor ${this.hostInfo.hostId}] `);
Copy link
Contributor

@joyc-bq joyc-bq Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this is a nit: but I think the whitespace should be added to be inside the logTopology method so the method doesnt have to pass in a string with a whitespace every time

crystall-bitquill
crystall-bitquill reviewed Jan 24, 2025
View reviewed changes
common/lib/host_list_provider/monitoring/cluster_topology_monitor.ts Outdated Show resolved Hide resolved
common/lib/host_list_provider/monitoring/cluster_topology_monitor.ts Outdated Show resolved Hide resolved
common/lib/host_list_provider/monitoring/cluster_topology_monitor.ts Outdated Show resolved Hide resolved
common/lib/plugin_service.ts
timeoutMs
);
if (updatedHostList) {
if (updatedHostList !== this.hosts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does order matter?

@joyc-bq
Copy link
Contributor

joyc-bq commented Jan 24, 2025

were the integration tests for readwritesplitting + failover also tested with failover2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crystall-bitquill crystall-bitquill crystall-bitquill left review comments

@joyc-bq joyc-bq joyc-bq left review comments

@karenc-bq karenc-bq karenc-bq left review comments

@sergiyvamz sergiyvamz Awaiting requested review from sergiyvamz

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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