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 error logging for crc32 validation failures. #563

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

why-arong
Copy link
Contributor

@why-arong why-arong commented Oct 18, 2023

Description

This PR add logging for situations where CRC32 validation does not pass.

By monitoring these failures, we can ensure data integrity throughout the application.

Co-authored-by: jaehyeonpy jaehyeonpy@gmail.com
Co-authored-by: davinc71998 davinc71998@gmail.com
Comment on lines 61 to 68
if byte_data == footer:
self._is_event_valid = True
else:
self._is_event_valid = False
logging.error(
f"An CRC32 has failed for the event type {self.event_type}, "
"indicating a potential integrity issue with the data."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if byte_data == footer:
self._is_event_valid = True
else:
self._is_event_valid = False
logging.error(
f"An CRC32 has failed for the event type {self.event_type}, "
"indicating a potential integrity issue with the data."
)
self._is_event_valid = True if byte_data == footer else False
if not self._is_event_valid:
logging.error(
f"An CRC32 has failed for the event type {self.event_type}, "
"indicating a potential integrity issue with the data."
)

@why-arong
I think this looks more readable, what do you think?
I think the code you wrote is good, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that code reads better!

How about error message? Is there any additional information we can provide besides 'self.event_type'??

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's enough

@why-arong
Copy link
Contributor Author

why-arong commented Oct 18, 2023

@sean-k1

logging.log(
logging.WARN,
"""
Before using MARIADB 10.5.0 and MYSQL 8.0.14 versions,
use python-mysql-replication version Before 1.0 version """,
)

How do you feel about not writing the code like logging.log(logging.ERROR, ~~~)? Unless I need to set the log level dynamically, I think logging.error(~~~) is more intuitive, so that's how I wrote it!

@sean-k1
Copy link
Collaborator

sean-k1 commented Oct 19, 2023

@sean-k1

logging.log(
logging.WARN,
"""
Before using MARIADB 10.5.0 and MYSQL 8.0.14 versions,
use python-mysql-replication version Before 1.0 version """,
)

How do you feel about not writing the code like logging.log(logging.ERROR, ~~~)? Unless I need to set the log level dynamically, I think logging.error(~~~) is more intuitive, so that's how I wrote it!

logging.error is more better👍

Copy link
Collaborator

@sean-k1 sean-k1 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks your hard work!

@sean-k1 sean-k1 merged commit 08a8fe3 into julien-duponchelle:main Oct 19, 2023
6 checks passed
@why-arong why-arong deleted the crc32_logging branch October 19, 2023 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants