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

[MDEV-15935] support redirection for MariaDB Connector J #134

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

GuuBu
Copy link
Contributor

@GuuBu GuuBu commented Mar 8, 2019

This pull request implements redirection protocol as discussed with community in https://jira.mariadb.org/browse/MDEV-15935.
The basic idea is leveraging OK packet last message to encapsulate the redirection information.

@ColinNg
Copy link

ColinNg commented May 16, 2019

It seems that the mirror where the AppVeyor Build it gets the MSI packages from, http://mariadb.mirrors.ovh.net/MariaDB/, is missing several versions:

10.2.21 (missing)
10.3.12 (missing)
10.1.37 (missing)
10.0.37 (missing)
5.5.62 (missing)

Whereas these versions are present on the mirror:
10.3.13 (present)
10.0.38 (present)
5.5.63 (present)

@GuuBu
Copy link
Contributor Author

GuuBu commented May 21, 2019

It seems that the mirror where the AppVeyor Build it gets the MSI packages from, http://mariadb.mirrors.ovh.net/MariaDB/, is missing several versions:
10.2.21 (missing)
10.3.12 (missing)
10.1.37 (missing)
10.0.37 (missing)
5.5.62 (missing)
Whereas these versions are present on the mirror:
10.3.13 (present)
10.0.38 (present)
5.5.63 (present)

Hi ColinNg,

About your comment, may I ask what action should I take for this to go on further progress? Thanks.

@ColinNg
Copy link

ColinNg commented May 21, 2019

@GuuBu Probably just need to do a rebase of your branch atop the latest master. Recently master had some commits which has fixed the AppVeyor build failure. You can then force-push your rebased branch, just like RobertDeRose did for PR #139.

@GuuBu
Copy link
Contributor Author

GuuBu commented May 31, 2019

Hi ColinNg,

Thanks. I've rebased the code and test has been passed. Can you help review the source code change. Thanks.

Best Regards,
Qianqian Bu

@vuvova
Copy link
Member

vuvova commented Sep 1, 2019

Can this be used now? Or is it useless until the server will be able to send redirection information?

@GuuBu
Copy link
Contributor Author

GuuBu commented Sep 3, 2019

Can this be used now? Or is it useless until the server will be able to send redirection information?

Hi @vuvova ,
It can be used now with Azure MySQL server. And yes, it is useless until server will be able to send redirection information.

Best Regards,
Qianqian Bu

@vuvova
Copy link
Member

vuvova commented Sep 3, 2019

Okay, so Azure MySQL server has it, correct?
Is it your fork of MySQL?
Do you plan to create a pull request for the MariaDB server part of this functionality?
How the server knows when to send a redirect request and to where?

@GuuBu
Copy link
Contributor Author

GuuBu commented Sep 11, 2019

@vuvova

Sorry for the late response. Please check the content below. Thanks.

Okay, so Azure MySQL server has it, correct?
Yes. In order to address customer needs for better latency, we implemented this following a previous discussion with Markus and Vladislav in this Jira thread: https://jira.mariadb.org/browse/MDEV-15935.

Is it your fork of MySQL?
We implemented this protocol in the middleware (proxy) and add neccessary support on MySQL server side which is our private repo

Do you plan to create a pull request for the MariaDB server part of this functionality?
Since the protocol has been discussed through, we can create a pull request to support redirection protocol for MariaDB server.

How the server knows when to send a redirect request and to where?
As Markus/Vladislav said in the Jira thread, the good part for this protocol is we could leverage existing MySQL protocol of OK Packet for K-V capabilities. Server will always send the redirect info within the OK packet for correct authentications and it would be client driver's choice to reconnect directly to the server.

Best Regards,
Qianqian Bu

@vuvova
Copy link
Member

vuvova commented Sep 12, 2019

It might be not a good idea to accept this pull request before server support. Without MariaDB server implementing redirects, this Connector/J feature cannot be properly tested, so it'll essentially be a dead code and will definitely stop working soon because of some unrelated changes, because the test suite will not be able to alert when the code is broken.

I think it make sense to get redirect support into MariaDB server and connectors at about the same time. Then we can test that they all work together and have automatic test suites to make sure that they will work in the future too.

@rusher
Copy link
Collaborator

rusher commented May 7, 2020

Server pull request MariaDB/server#1472 is now OK.
But we needs a Client implementation to ensure that before merging server PR.

This PR has been there since more than one year, and connection implementation has been rewritten and simplified since. Could you update PR to current codebase ? I will do that in a few weeks if not.

@GuuBu
Copy link
Contributor Author

GuuBu commented May 8, 2020

Hi @rusher ,
Thanks for reminding, I'll update the PR soon. I notice the OKPacket class is removed. There are several points update according to server side change, and since they are specific to OK packet, can I add the OKPacket class back?
Thanks.

@GuuBu
Copy link
Contributor Author

GuuBu commented May 20, 2020

Hi @rusher
I've updated the logic, can you help check? It seems the test failure is not relevant?

Best Regards,
Qianqian

@GuuBu
Copy link
Contributor Author

GuuBu commented May 20, 2020

I've built a server from PR MariaDB/server#1472, and the basic redirection connection test can pass now.

@GuuBu
Copy link
Contributor Author

GuuBu commented May 28, 2020

Hi @vuvova @bgrainger @rusher

I've updated the PR according to server side protocol, and add a new option for enableRedirect option. The enableRedirect has three choice now:
"off" will not use redirection,
"on" will enforce redirection (will fail if redirection is not available or redirection connection fails),
"preferred" will use redirection if it is available and the redirection succeed, otherwise, it will just the non-redirected connection.

Could you help check and review? Thanks.

Best Regards,
Qianqian Bu

try {

Pattern INFO_PATTERN =
Pattern.compile("^Location: mysql://\\[([^\\]:]+)\\]:([0-9]+)/\\?user=([^&]+)&ttl=([0-9]+)\\n");

Choose a reason for hiding this comment

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

Does this need any flexibility for future server-side changes? E.g., allow ttl "parameter" to be omitted, or allow additional parameters to be added?

Although, arguably, if additional parameters were added by the server, it might be safest to fail to parse the string on the client, as we can't guarantee that it would safe to simply ignore any extra parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From server side's change, the field ttl is always, so I think the code is safe. About the flexibility, I think maybe we we can handle it in future if new field and related logic is introduced, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants