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

Client session tracking #1092

Merged
merged 8 commits into from
Mar 9, 2020
Merged

Conversation

insom
Copy link
Contributor

@insom insom commented Dec 11, 2019

This adds support for Oracle/Percona MySQL 5.7+ session tracking to the client.

Docs on the function: https://dev.mysql.com/doc/refman/5.7/en/mysql-session-track-get-first.html

At Shopify, we wanted to be able to tell which GTID was generated by a given COMMIT so that when we are reading from delayed replicas we can be sure that a given replica has the data that we want on it. This is pretty much the exact flow that's described by the Oracle worklog entry describing the implementation of the feature

Example usage:

require 'mysql2'
c = Mysql2::Client.new(:host => "127.0.0.1", :username => "root", :password => "test1", :flags => "SESSION_TRACK", :database => "test");
c.query("SET SESSION session_track_gtids=OWN_GTID")
c.query("INSERT INTO test VALUES (1)");
gtid = c.session_track(Mysql2::Client::SESSION_TRACK_GTIDS)[0]

c2 = Mysql2::Client.new(:host => "replica.host")
c2.query("SELECT WAIT_FOR_EXECUTED_GTID_SET('#{gtid}')")
# waits forever, by default, probably not what people want
c2.query("SELECT * FROM test")

I mirrored the implementation of client.last_id -- I noticed that wasn't documented in the README.md so I hadn't added this, but if you have any suggestions about how or where to document this feature I'd appreciate that.

I believe that all the ifdefing is right to not compile this feature for MariaDB (which doesn't support it).

@insom insom closed this Dec 11, 2019
@insom insom reopened this Jan 7, 2020
@insom insom marked this pull request as ready for review January 7, 2020 15:31
@sodabrew sodabrew added this to the 0.6.0 milestone Jan 7, 2020
@sodabrew
Copy link
Collaborator

sodabrew commented Jan 7, 2020

Thank you!

Few things needed:

  • Unit tests (which should skip if MySQL < 5.7)
  • README updates to add a section about how to use this feature
  • Example code should use the init_command feature because it is works with reconnect to ensure the initial SQL is executed even if the connection drops and is transparently reconnected.
require 'mysql2'

c = Mysql2::Client.new(
    host: "127.0.0.1",
    username: "root",
    password: "test1",
    database: "test",
    flags: "SESSION_TRACK",
    init_command: "SET SESSION session_track_gtids=OWN_GTID")
c.query("INSERT INTO test VALUES (1)")
gtid = c.session_track(SESSION_TRACK_GTIDS).first

@@ -1613,6 +1641,21 @@ void init_mysql2_client() {
INT2NUM(0));
#endif

#ifdef CLIENT_SESSION_TRACK
rb_define_method(cMysql2Client, "session_track", rb_mysql_client_session_track, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The pattern I'd like to follow is to always define this method on the object, and within the method, if the feature is unavailable, to return Qnil.

@bibstha bibstha force-pushed the session-tracking branch 2 times, most recently from a7ada4b to aacf1ad Compare February 7, 2020 20:02
@bibstha
Copy link
Contributor

bibstha commented Feb 7, 2020

@sodabrew I've made the following changes

  1. Always defined session_track method and conditionally returns nil.
  2. Updated readme with information on how to call session_track method.

@bibstha
Copy link
Contributor

bibstha commented Feb 7, 2020

Also seems like the failing test is due to network failure. I'm unable to rerun the failing build, hum.

@bibstha
Copy link
Contributor

bibstha commented Feb 24, 2020

Hi @sodabrew, do you have some time to review this PR?

c = Mysql2::Client.new(
host: "127.0.0.1",
username: "root",
flags: "SESSION_TRACK",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example might be wrong? Should it be flags: Mysql2::Client::SESSION_TRACK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can pass all sorts of things to flags 😁 -- https://github.com/brianmario/mysql2/blob/master/lib/mysql2/client.rb#L56L57 -- so this will work. It might be better to encourage the use of the constant for style reasons though? The other examples do.

Allow compilation with mariadb-connector-c
@jeremy
Copy link
Contributor

jeremy commented Mar 7, 2020

This is a great feature. Thank you for contributing it.

@@ -1,6 +1,6 @@
require 'spec_helper'

RSpec.describe Mysql2::Client do
RSpec.describe Mysql2::Client do # rubocop:disable Metrics/BlockLength
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance to disable/extend this rubocop rule in specs only? rspec is usually bunch of huge blocks by design.

@sodabrew sodabrew merged commit cb9e941 into brianmario:master Mar 9, 2020
@sodabrew
Copy link
Collaborator

sodabrew commented Mar 9, 2020

Thanks again for this the support for this feature!

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

Successfully merging this pull request may close these issues.

6 participants