Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add properties in gtid event #586

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

Open
Bue-von-hon wants to merge 7 commits into julien-duponchelle:main
base: main
Choose a base branch
Loading
from Bue-von-hon:feature/add-properties-in-gtid-event

Conversation

@Bue-von-hon
Copy link

@Bue-von-hon Bue-von-hon commented Dec 1, 2023

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).

dongwook-chan reacted with thumbs up emoji
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?

dongwook-chan reacted with thumbs up emoji

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

dongwook-chan and sean-k1 reacted with thumbs up emoji

@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
edited
Loading

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

@Bue-von-hon Bue-von-hon Dec 4, 2023

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
Copy link
Author

Bue-von-hon commented Dec 5, 2023
edited
Loading

@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 reacted with thumbs up emoji

@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.
Co-authored-by: sean.k1 (Umhyunsik) <sean.k1@kakaoent.com>
@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
Co-authored-by: sean.k1 (Umhyunsik) <sean.k1@kakaoent.com>
@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

@sean-k1 sean-k1 Dec 6, 2023

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

@Bue-von-hon Bue-von-hon Dec 6, 2023

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. 👍

sean-k1 reacted with thumbs up emoji sean-k1 reacted with heart emoji
@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
Copy link
Collaborator

sean-k1 commented Dec 6, 2023
edited
Loading

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

Copy link
Collaborator

dongwook-chan commented Dec 17, 2023
edited
Loading

@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

Reviewers

@sean-k1 sean-k1 sean-k1 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /