You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/12/08 03:46:01 UTC
[GitHub] [shardingsphere] azexcy opened a new pull request, #22740: Improve MySQL incremental client reconnect and close
azexcy opened a new pull request, #22740:
URL: https://github.com/apache/shardingsphere/pull/22740
1. avoid `lastBinlogEvent` is null when init
2. triggers reconnection when timeout happened, no reconnection when manually stopped
Changes proposed in this pull request:
- Improve MySQL incremental client reconnect and close
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#discussion_r1043023564
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/MySQLClient.java:
##########
@@ -270,6 +280,10 @@ private final class MySQLBinlogEventHandler extends ChannelInboundHandlerAdapter
private AbstractBinlogEvent lastBinlogEvent;
Review Comment:
OK.
1, Could we use `@AllArgsConstructor` to replace constructor?
2, Could we add `volatile` for `lastBinlogEvent`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] azexcy commented on pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
azexcy commented on PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#issuecomment-1341951288
The RSA public key may contain blank lines at the end, just like below
```
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAwAsuZm+4c4DKg9gy1+Xk
+yitJPruAvakdQ8Zvigb3sO0WgbvHcmdl+SBUPwXKEgF4rxcRKVbJow9j2v7AyYm
jq3Y1HyqbWp9pmhZdwWHkmn41r2hn+qvBhCRYu1oCsuBwizSGHm3xrnKy416tjEY
zz9xQSyhZNJy+/L91KOY/xK20cm5QUBXCEV/Xi/OmfSq0d6WHan0R+F1iZi8uv0k
eaQVz/yCCRNpdVf+WSlAeba3G99ytd106IscjrmAHiafp+d+NkQFaMi0U8KZEKt1
KLTGAmIgBBDdCDsW6voxBYwWVxQKB5XT+WGpPmMGME64osBu6deil9j/0MWFoGkp
YQIDAQAB
-----END PUBLIC KEY-----
```
need remove it, otherwise the `java.lang.IllegalArgumentException: Illegal base64 character` will thrown
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#discussion_r1043019922
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/PasswordEncryption.java:
##########
@@ -107,7 +107,7 @@ private static RSAPublicKey parseRSAPublicKey(final String key) throws GeneralSe
private static byte[] formatKey(final String key) {
int start = key.indexOf("\n") + 1;
- int end = key.lastIndexOf("\n");
+ int end = key.endsWith("\n") ? key.substring(0, key.length() - 2).lastIndexOf("\n") : key.lastIndexOf("\n");
return key.substring(start, end).replace("\n", "").getBytes();
Review Comment:
1, OK. Could we add unit test for `parseRSAPublicKey`? Since it doesn't occur every time in E2E test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#discussion_r1043219831
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/PasswordEncryption.java:
##########
@@ -99,15 +99,15 @@ private static byte[] encryptWithRSAPublicKey(final byte[] source, final RSAPubl
}
private static RSAPublicKey parseRSAPublicKey(final String key) throws GeneralSecurityException {
- byte[] certificateData = Base64.getDecoder().decode(formatKey(key));
+ byte[] certificateData = Base64.getDecoder().decode(formatKey(key.trim()));
Review Comment:
It's better to put `key.trim()` in `formatKey` method, since it's the duty of `formatKey`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#discussion_r1042968895
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/MySQLClient.java:
##########
@@ -270,6 +280,10 @@ private final class MySQLBinlogEventHandler extends ChannelInboundHandlerAdapter
private AbstractBinlogEvent lastBinlogEvent;
Review Comment:
Could `lastBinlogEvent` be final and simplify constructor?
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/PasswordEncryption.java:
##########
@@ -107,7 +107,7 @@ private static RSAPublicKey parseRSAPublicKey(final String key) throws GeneralSe
private static byte[] formatKey(final String key) {
int start = key.indexOf("\n") + 1;
- int end = key.lastIndexOf("\n");
+ int end = key.endsWith("\n") ? key.substring(0, key.length() - 2).lastIndexOf("\n") : key.lastIndexOf("\n");
return key.substring(start, end).replace("\n", "").getBytes();
Review Comment:
1, When it needs RSA?
2, Could we just use `key.trim()` to replace `key.substring(start, end)`?
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/MySQLClient.java:
##########
@@ -157,6 +158,7 @@ public synchronized void subscribe(final String binlogFileName, final long binlo
dumpBinlog(binlogFileName, binlogPosition, queryChecksumLength());
log.info("subscribe binlog file: {}, position: {}", binlogFileName, binlogPosition);
reconnectTimes.set(0);
+ running = true;
Review Comment:
Could we put `reconnectTimes` and `running` reset logic to `connect` method instead of `subscribe` method?
Since they're related to connection but not subscription.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] azexcy commented on a diff in pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
azexcy commented on code in PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740#discussion_r1042987027
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/MySQLClient.java:
##########
@@ -270,6 +280,10 @@ private final class MySQLBinlogEventHandler extends ChannelInboundHandlerAdapter
private AbstractBinlogEvent lastBinlogEvent;
Review Comment:
It's update at `channelRead`, should we add final and use setter update lastBinlogEvent?
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/PasswordEncryption.java:
##########
@@ -107,7 +107,7 @@ private static RSAPublicKey parseRSAPublicKey(final String key) throws GeneralSe
private static byte[] formatKey(final String key) {
int start = key.indexOf("\n") + 1;
- int end = key.lastIndexOf("\n");
+ int end = key.endsWith("\n") ? key.substring(0, key.length() - 2).lastIndexOf("\n") : key.lastIndexOf("\n");
return key.substring(start, end).replace("\n", "").getBytes();
Review Comment:
the `publicKeyRequested` property determines whether rsa is required
```
private void handleCachingSha2Auth(final ChannelHandlerContext ctx, final MySQLAuthMoreDataPacket authMoreData) {
// how caching_sha2_password works: https://dev.mysql.com/doc/dev/mysql-server/8.0.11/page_caching_sha2_authentication_exchanges.html#sect_caching_sha2_info
if (!publicKeyRequested) {
if (PERFORM_FULL_AUTHENTICATION == authMoreData.getPluginData()[0]) {
publicKeyRequested = true;
ctx.channel().writeAndFlush(new MySQLAuthSwitchResponsePacket(authMoreData.getSequenceId() + 1, new byte[]{REQUEST_PUBLIC_KEY}));
}
} else {
ctx.channel().writeAndFlush(new MySQLAuthSwitchResponsePacket(authMoreData.getSequenceId() + 1,
PasswordEncryption.encryptWithRSAPublicKey(password, seed,
serverInfo.getServerVersion().greaterThanOrEqualTo(8, 0, 5) ? "RSA/ECB/OAEPWithSHA-1AndMGF1Padding" : "RSA/ECB/PKCS1Padding",
new String(authMoreData.getPluginData()))));
}
}
```
2. seem i can use `trim()` before passing in the key parameter
##########
kernel/data-pipeline/dialect/mysql/src/main/java/org/apache/shardingsphere/data/pipeline/mysql/ingest/client/MySQLClient.java:
##########
@@ -157,6 +158,7 @@ public synchronized void subscribe(final String binlogFileName, final long binlo
dumpBinlog(binlogFileName, binlogPosition, queryChecksumLength());
log.info("subscribe binlog file: {}, position: {}", binlogFileName, binlogPosition);
reconnectTimes.set(0);
+ running = true;
Review Comment:
Ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz merged pull request #22740: Improve MySQL incremental client reconnect and close
Posted by GitBox <gi...@apache.org>.
sandynz merged PR #22740:
URL: https://github.com/apache/shardingsphere/pull/22740
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org