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

Add properties in gtid event #586

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

Conversation

Bue-von-hon
Copy link

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
  • New feature
  • Documentation update
  • Other (please specify below)

Checklist

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

Tests

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

@sean-k1
Copy link
Collaborator

sean-k1 commented Dec 1, 2023

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

@Bue-von-hon
Copy link
Author

@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 Bue-von-hon marked this pull request as draft December 1, 2023 13:44
self.assertIsInstance(gtid_event.sid, bytes)
self.assertIsInstance(gtid_event.gno, int)
self.assertIsInstance(gtid_event.lt_type, int)
self.assertIsInstance(gtid_event.last_committed, int)
Copy link
Collaborator

@sean-k1 sean-k1 Dec 2, 2023

Choose a reason for hiding this comment

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

Upper mysql 5.7 version has this variable so pytest failed.

  1. base.py make method isMySQL57AndMore
  2. Test setUp method use isMySQL57AndMore* before start Test

Copy link
Author

Choose a reason for hiding this comment

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

Resolves it!

@Bue-von-hon Bue-von-hon marked this pull request as ready for review December 4, 2023 04:08
@Bue-von-hon
Copy link
Author

Bue-von-hon commented Dec 5, 2023

@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?

@Bue-von-hon Bue-von-hon marked this pull request as draft December 6, 2023 03:27
taehun.kim and others added 4 commits December 6, 2023 12:36
Motivation:
GtidEvent is aleady implemented but there are missing 6 properties.
If we respect these properties, user can use GtidEvent with more convenient.

Motivation:
- 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.
@Bue-von-hon Bue-von-hon force-pushed the feature/add-properties-in-gtid-event branch from f200a56 to e6b2a54 Compare December 6, 2023 03:38
@Bue-von-hon Bue-von-hon marked this pull request as ready for review December 6, 2023 07:15
self.execute(
"CREATE TABLE IF NOT EXISTS test (id INT AUTO_INCREMENT PRIMARY KEY, name VARCHAR(255))"
)
gtid_event = self.stream.fetchone()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should grep FormatEvent too

Now gtid_event assign FormatDescriptionEvent

Copy link
Author

Choose a reason for hiding this comment

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

After i grep FormatDescriptionEvent, test was passed in my local env. 👍

@Bue-von-hon Bue-von-hon marked this pull request as draft December 6, 2023 08:03
@Bue-von-hon Bue-von-hon marked this pull request as ready for review December 6, 2023 08:11
@sean-k1
Copy link
Collaborator

sean-k1 commented Dec 6, 2023

@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 Bue-von-hon marked this pull request as draft December 11, 2023 04:17
@Bue-von-hon
Copy link
Author

@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 🙏

@Bue-von-hon Bue-von-hon marked this pull request as ready for review December 11, 2023 06:36
@sean-k1
Copy link
Collaborator

sean-k1 commented Dec 13, 2023

@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
Copy link
Collaborator

dongwook-chan commented Dec 17, 2023

@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;
  }

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.

3 participants