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

[FLINK-34689][MySQL][Feature] check binlog_row_value_options #3148

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

SML0127
Copy link
Contributor

@SML0127 SML0127 commented Mar 15, 2024

Implemented functino to check binlog_row_value_optoins within MySqlValidator.

When binlog_row_value_optoins is set to PARTIAL_JSON,
the update operator remains as Update_rows_partial.
pasted_image_at_2024-03-07T03_21_08 548Z

Flink CDC does not parse this event because Update_row_partial binlog event is mapped to PARTIAL_UPDATE_ROWS_EVENT and Flink CDC do not handle that event type. Therefore, some update query-related binary logs are lost.

So, we have to check binlog_row_value_optoins before starting.

Implemented checkBinlogRowValueOptions

private final MySqlSourceConfig sourceConfig;
    public void validate() {
        checkVersion(connection);
        checkBinlogFormat(connection);
        checkBinlogRowImage(connection);
        checkBinlogRowValueOptions(connection);
        checkTimeZone(connection);
    } catch (SQLException ex) {
        throw new TableException(
    }
}

/** Check whether the binlog row value options is empty. */
private void checkBinlogRowValueOptions(JdbcConnection connection) throws SQLException {
    String rowValueOptions =
           connection
                    .queryAndMap(
                            "SHOW GLOBAL VARIABLES LIKE 'binlog_row_value_options'",
                            rs -> rs.next() ? rs.getString(2) : "")
                    .trim()
                    .toUpperCase();
    // This setting was introduced in MySQL 8.0+ with default of empty string ''
    // For older versions, assume empty string ''
    if (!rowValueOptions.equals(BINLOG_ROW_VALUE_OPTIONS)) {
        throw new ValidationException(
                String.format(
                       "The MySQL server is configured with binlog_row_value_options %s rather than %s, which is "
                                + "required for this connector to work properly. Change the MySQL configuration to use a "
                                + "binlog_row_image='' and restart the connector.",
                        rowValueOptions, BINLOG_ROW_VALUE_OPTIONS));
    }
}

@SML0127
Copy link
Contributor Author

SML0127 commented Mar 15, 2024

@SML0127 SML0127 changed the title [MySQL][Feature] check binlog_row_value_optoins [FLINK-34689][MySQL][Feature] check binlog_row_value_optoins Mar 15, 2024
@SML0127
Copy link
Contributor Author

SML0127 commented Mar 16, 2024

@leonardBang , @ruanhang1993 CC

@leonardBang
Copy link
Contributor

Thanks @SML0127 for the contribution, @ruanhang1993 Would you like to help review this PR?

@SML0127
Copy link
Contributor Author

SML0127 commented Mar 21, 2024

@ruanhang1993 PTAL

Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

@SML0127 Hi. Sorry for the late review. I left a little comment. Thanks~

"The MySQL server is configured with binlog_row_value_options %s rather than %s, which is "
+ "required for this connector to work properly. Change the MySQL configuration to use a "
+ "binlog_row_image='' and restart the connector.",
rowValueOptions, BINLOG_ROW_VALUE_OPTIONS));
Copy link
Contributor

@ruanhang1993 ruanhang1993 Apr 10, 2024

Choose a reason for hiding this comment

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

I think this error message should be changed as follows.

The MySQL server is configured with binlog_row_value_options=%s, which is possible to cause losing some binlog events for the mysql cdc connector. Please remove the binlog_row_value_options setting in the MySQL server and rerun the job. See more details at https://dev.mysql.com/doc/refman/8.0/en/replication-features-json.html.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I do not understand wrong, we do not need to change the binlog_row_image setting. Am I right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do not understand wrong, we do not need to change the binlog_row_image setting. Am I right ?

@ruanhang1993
I updated log message.

Yes. we do not need to change both settings binlog_row_image and binlog_row_value_options.

Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

Hi, @SML0127.
I left some comments. Please take a look, thanks~

@@ -50,6 +50,7 @@ public class MySqlValidator implements Validator {

private static final String BINLOG_FORMAT_ROW = "ROW";
private static final String BINLOG_FORMAT_IMAGE_FULL = "FULL";
private static final String BINLOG_ROW_VALUE_OPTIONS = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static final String BINLOG_ROW_VALUE_OPTIONS = "";
private static final String DEFAULT_BINLOG_ROW_VALUE_OPTIONS = "";

connection
.queryAndMap(
"SHOW GLOBAL VARIABLES LIKE 'binlog_row_value_options'",
rs -> rs.next() ? rs.getString(2) : "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rs -> rs.next() ? rs.getString(2) : "")
rs -> rs.next() ? rs.getString(2) : DEFAULT_BINLOG_ROW_VALUE_OPTIONS)

.toUpperCase();
// This setting was introduced in MySQL 8.0+ with default of empty string ''
// For older versions, assume empty string ''
if (!rowValueOptions.equals(BINLOG_ROW_VALUE_OPTIONS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!rowValueOptions.equals(BINLOG_ROW_VALUE_OPTIONS)) {
if (!BINLOG_ROW_VALUE_OPTIONS.equals(rowValueOptions)) {

if (!rowValueOptions.equals(BINLOG_ROW_VALUE_OPTIONS)) {
throw new ValidationException(
String.format(
"The MySQL server is configured with binlog_row_value_options=%s, which is possible to cause losing some binlog events"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The MySQL server is configured with binlog_row_value_options=%s, which is possible to cause losing some binlog events"
"The MySQL server is configured with binlog_row_value_options=%s, which is possible to cause losing some binlog events "

"The MySQL server is configured with binlog_row_value_options=%s, which is possible to cause losing some binlog events"
+ "for the mysql cdc connector. Please remove the binlog_row_value_options setting in the MySQL server and rerun the job."
+ "See more details at https://dev.mysql.com/doc/refman/8.0/en/replication-features-json.html.",
rowValueOptions, BINLOG_ROW_VALUE_OPTIONS));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rowValueOptions, BINLOG_ROW_VALUE_OPTIONS));
rowValueOptions));

@SML0127
Copy link
Contributor Author

SML0127 commented Apr 11, 2024

Hi, @SML0127. I left some comments. Please take a look, thanks~

@ruanhang1993
Thank you for your comments!
I have applied all of your comments to the code.

Comment on lines 51 to 52
private static final String BINLOG_FORMAT_ROW = "ROW";
private static final String BINLOG_FORMAT_IMAGE_FULL = "FULL";
Copy link
Contributor Author

@SML0127 SML0127 Apr 11, 2024

Choose a reason for hiding this comment

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

@ruanhang1993
How about changing the variable names BINLOG_FORMAT_ROW, BINLOG_FORMAT_IMAGE_FULL to DEFAULT_BINLOG_FORMAT, DEFAULT_BINLOG_ROW_FORMAT?

Suggested change
private static final String BINLOG_FORMAT_ROW = "ROW";
private static final String BINLOG_FORMAT_IMAGE_FULL = "FULL";
private static final String DEFAULT_BINLOG_FORMAT = "ROW";
private static final String DEFAULT_BINLOG_ROW_FORMAT = "FULL";

In mysql 8, default values for the binlog_format and binlog_row_image options are ROW and full, respectively

Copy link
Contributor

@ruanhang1993 ruanhang1993 Apr 11, 2024

Choose a reason for hiding this comment

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

Hi, @SML0127 .
I think we could not change these fields' names. These settings are checked by the ROW or FULL values.
But the new DEFAULT_BINLOG_ROW_VALUE_OPTIONS is used as the default value for mysql 5.7. And it is to check whether this setting is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your explanation, @ruanhang1993
I undertand your responses. Do you have any other suggestions?

Copy link
Contributor

@ruanhang1993 ruanhang1993 left a comment

Choose a reason for hiding this comment

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

LGTM.

@PatrickRen PatrickRen changed the title [FLINK-34689][MySQL][Feature] check binlog_row_value_optoins [FLINK-34689][MySQL][Feature] check binlog_row_value_options Apr 11, 2024
Copy link
Contributor

@PatrickRen PatrickRen left a comment

Choose a reason for hiding this comment

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

@SML0127 Thanks for the contribution! LGTM

@PatrickRen PatrickRen merged commit af7665d into apache:master Apr 12, 2024
13 checks passed
wuzhenhua01 pushed a commit to wuzhenhua01/flink-cdc-connectors that referenced this pull request Aug 4, 2024
ChaomingZhangCN pushed a commit to ChaomingZhangCN/flink-cdc that referenced this pull request Jan 13, 2025
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.

4 participants