-
Notifications
You must be signed in to change notification settings - Fork 256
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
Redirection client support #137
base: 3.1
Are you sure you want to change the base?
Redirection client support #137
Conversation
} | ||
} | ||
|
||
int redirection_set_connection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it for future use? It just returns 0, without setting any connection?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't mean to use set_connection... But without defining a dummy set_connection the client will crash in mthd_my_send_cmd:
if (IS_CONNHDLR_ACTIVE(mysql))
{
result= mysql->extension->conn_hdlr->plugin->set_connection(mysql, command, arg, length, skipp_check, opt_arg);
...
}
Looks to me that a null check is missing here(as the check exists for other plugin members like reconnect and reset); or do I get it wrong and set_connection is a must-have member for a connection plugin? If so, what is it supposed to do?
#include "redirection_utility.h" | ||
|
||
// Note that key, host and user and malloced together with Redirection_Entry | ||
struct Redirection_Entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you use LIST (ma_list.c) ? For an example check mysql->stmts and ma_invalidate_stmts() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really think LIST is a good abstraction here... Redirection cache is an LRU cache indexed by a string-value key, so I expect key addressing like C++ dictionaries which LIST justifiably does not support. Besides I need to move a list node to head or tail of that list(so that the least recent used record can be found immediately), which LIST does not support either.
I tried writing the cache as a LIST of key-indexed Redirection Entries and got a getter function like below:
struct Redirection_Entry *cur_entry;
struct Redirection_Entry *ret_entry;
/* find the entry by key */
LIST *cur = redirection_cache_head;
while (cur)
{
cur_entry = (struct Redirection_Entry *)(cur->data);
if (cur_entry && cur_entry->key)
{
if (!strcmp(cur_entry->key, key))
{
ret_entry = cur_entry;
break;
}
cur = cur->next;
}
else break;
}
if (ret_entry)
{
/* move to list head */
if (redirection_cache_head != cur)
{
if (redirection_cache_tail == cur){
redirection_cache_tail = cur->prev;
}
cur->prev->next = cur->next;
if (cur->next)
{
cur->next->prev = cur->prev;
}
cur->next = redirection_cache_head;
cur->prev = 0;
redirection_cache_head = cur;
}
}
It occurs to me I didn't gain much from the LIST abstraction other than the prev and next pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, it might be a good idea to implement redirection cache as an array instead of a list. Array design is not ideal in that finding the least recent used cache entry takes O(n) time instead of O(1), but compared to time spent on network requests the overhead should be negligible. On the other hand, with an array of records there's no more complicated manipulation of head, tail, next and prev pointers, and (hopefully) my code will be much easier to read. May I have your opinion on the idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 9EOR9, I've re-implemented redirection cache as an array. Would you mind taking a look at the latest changes?
Two comments:
let's use one of those, and not introduce a new one. The second one looks like very close to what you have, your example would be
Aside from these small changes, looks pretty good |
@@ -3053,6 +3053,9 @@ mysql_optionsv(MYSQL *mysql,enum mysql_option option, ...) | |||
mysql->options.extension->connect_attrs_len= 0; | |||
} | |||
break; | |||
case MARIADB_OPT_USE_REDIRECTION: | |||
OPT_SET_EXTENDED_VALUE(&mysql->options, redirection_mode, *(enable_redirect*)arg1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, don't do that. sizeof(enum) is not portable, it depends even on compilation options. Use a stable portable type for passing arguments. For example, uint
Another option would to set options in plugin->options, e.g.
|
Background
This pull request implements client side redirection functionality for mariadb connector C. Server side changes are made in https://github.com/MariaDB/server/pull/1472.
Work Done
For a brief view of what this pull request do consider using mariadb connector C to connect to a MySQL/MariaDB server when the server is placed behind some gateway/proxy who shadows the real address of the server.
Before this pull request we must connect to server through gateway and ask gateway to forward our requests to backend server; such forwarding introduces extra latency.
After this pull request(combined with the server request mentioned above), when redirection feature is enabled and a connection from gateway to server is established, server will send a string indicating its real address in the OK packet. The OK packet is forwarded by gateway to client, and client will make use of such information to set up a connection to server(redirect itself to server instead of to gateway).
With redirection two connection attempts will be made from client to set up a connection, whereas subsequent requests will have reduced latency; this pull request implements an in-memory cache that stores "real server addresses" for connections established, so that only one connection attempt needs to be made when connecting to a known server.
Design
The client side redirection workflow is implemented as a dynamic connection plugin. Users can play with redirection feature only when the plugin is loaded and used. The redirection workflow is intuitive: the client connects to gateway, retrieves and parses redirection info, uses such info to connect to backend server, and returns the redirected connection to caller.
An option named redirection_mode is added to mysql.options with ON, OFF and PREFERRED as acceptable value. The behaviour of redirection plugin depends on value of redirection_mode:
With redirection_mode ON, redirection is enforced on client side. If server does not send redirection info (the real address), or connection attempt to server using redirection info fails, mysql_real_connect reports failure.
With redirection_mode PREFERRED, client will fallback to gateway connection when redirection workflow fails for some reason.
With redirection_mode OFF client works exactly as it did before this pull request.
An in-memory redirection cache is initialized when plugin is loaded. On each successful redirection connection it stores the redirection info identified by original server host, user and port. When a new connection is to be established, the cache is searched for a matching redirection record. The client will make use of stored redirection info to setup connection to host whenever possible, and will fallback to normal redirection workflow only when no matching record exists or connection cannot be established using stored redirection info for this server.