Add properties in gtid event

Bue-von-hon commented 11 months ago

Description

Motivation: GtidEvent is aleady implemented but there are missing 6 properties. If we respect these properties, user can use GtidEvent with more convenient.

Motification:

Fix(event.py): Add two properties.
Add(test_basic.py): Add test for GtidEvent.

Result: for now on, user can use read_immediate_commit_timestamp and read_original_commit_timestamp properties.

Type of Change

[ ] Bug fix
[X] New feature
[ ] Documentation update
[ ] Other (please specify below)

Checklist

[X] I have read and understood the CONTRIBUTING.md document.
[X] I have checked there aren't any other open Pull Requests for the desired changes.
[X] This PR resolves an issue #[Issue Number Here].

Tests

[X] All tests have passed.
[X] New tests have been added to cover the changes. (describe below if applicable).

sean-k1 commented 11 months ago

@Bue-von-hon Thanks for contributing! Can you give me the url you referenced to check the code?

Bue-von-hon commented 11 months ago

@sean-k1 here is the official mysql documentation and the url of the cpp implementation that I referenced.

Please see the Binary Format section in Mysql: https://dev.mysql.com/doc/dev/mysql-server/8.1.0/classbinary__log_1_1Gtid__event.html

You can see the binary format starting at line 428 of the CPP implementation. CPP: https://github.com/mysql/mysql-server/blob/ca51ab1529a9ec4d73b368df15b65534485a617d/libbinlogevents/src/control_events.cpp#L417

Bue-von-hon commented 11 months ago

@sean-k1 Leave a comment about the test. PTAL🙏

Summary: The test is failing due to a mismatch between the mysql version found in the isMySQL57AndMore method and the mysql version in the init method of the BinLogEvent class.

Details: Upon investigating the reason for the test failure, I found that the init method of the BinLogEvent class written in event.py is not getting the correct mysql version information. The init method simply returns (0, 0, 0), which is set to the default value, as the version information.

In the isMySQL57AndMore method of the PyMySQLReplicationTestCase class written in base.py, I am getting the correct mysql version information (8.1 in my case).

So, the reason for the test failure is as follows The isMySQL57AndMore method passes and the test runs, but because the init method returns mysql incorrect version information, The following logic written in line 107 of the init method of the GtidEvent class does not set the last_committed, sequence_number properties.

if self.mysql_version >= (5, 7):
    self.last_committed = struct.unpack("<Q", self.packet.read(8))[0]
    self.sequence_number = struct.unpack("<Q", self.packet.read(8))[0]

p.s. I'm not sure if I should fix this bug in this PR. Why not create a new issue and fix it?

sean-k1 commented 11 months ago

@Bue-von-hon https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-1.html#:~:text=databases.%20(WL%20%234618)-,Replication,-%3A%20An%20infrastructure Immediate commit timestamp original commit timestamp are introduced in MySQL-8.0.1 so Test Failed in 5.7 env

Bue-von-hon commented 10 months ago

@Bue-von-hon https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-1.html#:~:text=databases.%20(WL%20%234618)-,Replication,-%3A%20An%20infrastructure Immediate commit timestamp original commit timestamp are introduced in MySQL-8.0.1 so Test Failed in 5.7 env

I have read the mysql release notes you attached. Thanks to @sean-k1, I was able to improve the tests to respect the mysql version information. I ran the test on my local environment against mysql 8.1 and 5.7 versions and found that it passes. PTAL 🙏

sean-k1 commented 10 months ago

@Bue-von-hon

5.7 users will get an error and terminate the process if they use this version I'm asking 5.7 users to write below 1.00, but I don't think it's a good idea to at least have the process terminate in error.

I'm not sure if branching based on myql version is a good idea either, so I'll think about it a bit more. @dongwook-chan What do you think?

dongwook-chan commented 10 months ago

@Bue-von-hon @sean-k1 I do agree that branching based on MySQL version isn't a good idea. It's almost always best to stick to the implementations in mysql-server. In that sense, we need python-mysql-replication equivalent of mysql-server 's can_read. We need to check how many bytes we have left to read and branch accordingly.

I have already created a similar method bytes_to_read in python-mysql-replication but I would have to say the implementation isn't necessarily elegant. So you're welcome to modify it or create a new one.

In case you haven't configured mysql-server locally, refer to following GitHub links and code blocks for details on the implementation of can_read in mysql-server.

can_read

  /**
    Returns if the Event_reader can read a given amount of bytes from cursor
    position.

    @param bytes the amount of bytes expected to be read.

    @retval true if the Event_reader can read the specified amount of bytes.
    @retval false if the Event_reader cannot read the specified amount of bytes.
  */
  bool can_read(unsigned long long bytes) {
    return (available_to_read() >= bytes);
  }

available_to_read

  /**
    Returns the amount of bytes still available to read from cursor position.

    @return the amount of bytes still available to read.
  */
  unsigned long long available_to_read() {
    BAPI_ASSERT(position() <= m_limit);
    return m_limit - position();
  }

position

  /**
    Returns the current Event_reader cursor position in bytes.

    @retval m_limit if cursor position is invalid.
    @retval position current Event_reader cursor position (if valid).
  */
  unsigned long long position() {
    return m_ptr >= m_buffer ? m_ptr - m_buffer : m_limit;
  }

julien-duponchelle / python-mysql-replication