You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "Netter99 (via GitHub)" <gi...@apache.org> on 2023/04/06 12:44:29 UTC
[GitHub] [shardingsphere] Netter99 opened a new pull request, #25036: Improve properties verification of TrafficAlgorithm #24754
Netter99 opened a new pull request, #25036:
URL: https://github.com/apache/shardingsphere/pull/25036
Fixes #24754.
Changes proposed in this pull request:
-
---
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] 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.
- [x] 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] RaigorJiang commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501838415
@Netter99 Merged, thank you!
--
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] RaigorJiang commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1161462371
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -41,8 +40,8 @@ public void init(final Properties props) {
ShardingSpherePreconditions.checkState(props.containsKey(REGEX_PROPS_KEY),
() -> new SegmentTrafficAlgorithmInitializationException(SQLRegexTrafficAlgorithm.class.getName(), String.format("%s cannot be null", REGEX_PROPS_KEY)));
regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
- ShardingSpherePreconditions.checkState(Strings.isNullOrEmpty(String.valueOf(regex)),
- () -> new SegmentTrafficAlgorithmInitializationException(SQLRegexTrafficAlgorithm.class.getName(), "regex must be not null or empty"));
+ ShardingSpherePreconditions.checkState(!Strings.isNullOrEmpty(String.valueOf(regex)),
Review Comment:
Hi @Netter99 @Pace2Car
I'm wondering if the result of `props.getProperty(REGEX_PROPS_KEY)` is null, will `Pattern.compile` throw an exception?
--
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] RaigorJiang commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1161396492
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -41,8 +44,11 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
- Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
+ ShardingSpherePreconditions.checkState(props.containsKey(SQL_PROPS_KEY),
+ () -> new SegmentTrafficAlgorithmInitializationException(SQLMatchTrafficAlgorithm.class.getName(), String.format("%s cannot be null", SQL_PROPS_KEY)));
sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ ShardingSpherePreconditions.checkState(Strings.isNullOrEmpty(String.valueOf(sql)),
Review Comment:
Is the expectation here is sql null or empty?
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501324807
@Pace2Car , thanks for your patient.
I have a question: do I need to reply to your message after every submission, or will your email automatically notify you once the submission is completed?
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501712041
@Netter99 Another PR affected this CI failurem, please merge master and push.
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501724975
> @Netter99 Another PR affected this CI failurem, please merge master and push.
Sorry, I don't understand what this mean. I delete the local and remote 'temp' branch. I am not sure whether it works or not.
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160499819
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
Review Comment:
`assertInitWithIllegalProps()` is better.
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160496007
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
Review Comment:
Please check if the code was pushed successfully, I see no modification.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
Review Comment:
Please check if the code was pushed successfully, I see no modification.
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501328922
> @Pace2Car , thanks for your patient. I have a question: do I need to reply to your message after every submission, or will your email automatically notify you once the submission is completed?
You're welcome : )
The email will be automatically notified, and you can also click `resolved` in the comments.
BTW: Run `./mvnw checkstyle:check -T1C` and `./mvnw -T1C -B -ntp clean install -Dmaven.javadoc.skip=true -Djacoco.skip=true` before each push
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501741933
> > Sorry, I don't understand what this mean. I delete the local and remote 'temp' branch. I am not sure whether it works or not.
>
> If you have prepared the environment according to the developer guide run
>
> ```
> git checkout yourBranch
> git merge apache/master
> git push origin yourBranch
> ```
>
> Replace `yourBranch` to your real branch(the branch that initiated this PR) name
I run the command as you say, it appears like this.
If there are still conflicts, may I close the current PR and make a new one?
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160498829
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlMatchAlgorithm = (SQLMatchTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_MATCH",
Review Comment:
code `sqlMatchAlgorithm = (SQLMatchTrafficAlgorithm)` is unnecessary.
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501532128
> @Pace2Car Please check again for my modification.
@Netter99 Please add license comments for SegmentTrafficAlgorithmInitializationException.java
--
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] Netter99 commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160447553
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLRegexMatch() {
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "TRUNCATE TABLE `t_order` ")));
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlRegexAlgorithm = (SQLRegexTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_REGEX",
Review Comment:
I have made some 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] Netter99 commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160506671
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
Review Comment:
check
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501487438
@Pace2Car Please check again for my modification.
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501841688
@Pace2Car Thanks for your patient guidance!
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501558434
@Pace2Car Thanks, please check again.
--
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] RaigorJiang merged pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang merged PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036
--
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] Netter99 commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160447967
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(sqlValue) || sqlValue.length() == 0), "%s cannot be empty.", SQL_PROPS_KEY);
Review Comment:
I have made some changes, please check.
--
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] Netter99 commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160506671
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
Review Comment:
check
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160473507
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ ShardingSpherePreconditions.checkState(Strings.isNullOrEmpty(String.valueOf(sql)),
+ () -> new SegmentTrafficAlgorithmInitializationException(SQLMatchTrafficAlgorithm.class.getName(), "SQL is not null or empty"));
Review Comment:
message `"sql must be not null or empty"` is better.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -37,6 +40,8 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
Review Comment:
as same.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
Review Comment:
Please use `ShardingSpherePreconditions` to replace `Preconditions`, Thank you.
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlMatchAlgorithm = (SQLMatchTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_MATCH",
Review Comment:
Please resolve this, and why was the test case for Regex removed?
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/exception/segment/SegmentTrafficAlgorithmInitializationException.java:
##########
@@ -0,0 +1,11 @@
+package org.apache.shardingsphere.traffic.exception.segment;
+
+import org.apache.shardingsphere.infra.util.exception.external.sql.sqlstate.XOpenSQLState;
+import org.apache.shardingsphere.traffic.exception.TrafficException;
+
+public class SegmentTrafficAlgorithmInitializationException extends TrafficException {
+
+ public SegmentTrafficAlgorithmInitializationException(final String methodName, final String reason) {
+ super(XOpenSQLState.INVALID_PARAMETER_VALUE, 1, "Segmentation traffic algorithm `%s` initialization failed, reason is: %s.", methodName, reason);
Review Comment:
use `XOpenSQLState.GENERAL_ERROR` and `98` as errorCode.
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160497387
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -37,6 +40,8 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
+ ShardingSpherePreconditions.checkState(Strings.isNullOrEmpty(String.valueOf(regex)),
+ () -> new SegmentTrafficAlgorithmInitializationException(SQLRegexTrafficAlgorithm.class.getName(), "sql must be not null or empty"));
Review Comment:
Please fix the message, replace `sql` to `regex`.
--
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] RaigorJiang commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "RaigorJiang (via GitHub)" <gi...@apache.org>.
RaigorJiang commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501326576
Please fix checkstyle:
```
Error: src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:[21,8] (imports) UnusedImports: Unused import - com.google.common.base.Preconditions.
```
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501734058
> Sorry, I don't understand what this mean. I delete the local and remote 'temp' branch. I am not sure whether it works or not.
If you have prepared the environment according to the developer guide
run
```
git checkout yourBranch
git merge apache
git push origin yourBranch
```
Replace `yourBranch` to your real branch(the branch that initiated this PR) name
--
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] Netter99 commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160490642
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
Review Comment:
It's wired, I have made the modification in the last PR.
I make the review as your advice again.
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1161245791
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLRegexMatch() {
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "TRUNCATE TABLE `t_order` ")));
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
}
+
+ @Test
+ void assertInitWithIllegalProps() {
Review Comment:
This should be a test case for regex, some parameters don't look right.
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501546011
@Netter99 The license needs to be declared at the top using comments, like
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
```
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160411844
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(sqlValue) || sqlValue.length() == 0), "%s cannot be empty.", SQL_PROPS_KEY);
Review Comment:
Use `ShardingSpherePreconditions` to replace `Preconditions`, and then define an accurate exception class, refer to #24939
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(sqlValue) || sqlValue.length() == 0), "%s cannot be empty.", SQL_PROPS_KEY);
Review Comment:
Use `Strings.isNullOrEmpty()` of guava is recommended.
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLRegexMatch() {
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "TRUNCATE TABLE `t_order` ")));
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlRegexAlgorithm = (SQLRegexTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_REGEX",
Review Comment:
as same.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -42,7 +43,9 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
- sql = getExactlySQL(props.getProperty(SQL_PROPS_KEY));
+ String sqlValue = props.getProperty(SQL_PROPS_KEY);
Review Comment:
This line is unnecessary and can be left as it is.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -36,7 +37,9 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
- regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
+ String regexValue = props.getProperty(REGEX_PROPS_KEY);
Review Comment:
as same.
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithm.java:
##########
@@ -36,7 +37,9 @@ public final class SQLRegexTrafficAlgorithm implements SegmentTrafficAlgorithm {
@Override
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(REGEX_PROPS_KEY), "%s can not be null.", REGEX_PROPS_KEY);
- regex = Pattern.compile(props.getProperty(REGEX_PROPS_KEY));
+ String regexValue = props.getProperty(REGEX_PROPS_KEY);
+ Preconditions.checkArgument(!(StringUtils.isEmpty(regexValue) || regexValue.length() == 0), "%s cannot be empty.", REGEX_PROPS_KEY);
Review Comment:
as same.
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertThrowExceptionWhenWithIllegalInit() {
+ assertThrows(IllegalArgumentException.class, () -> sqlMatchAlgorithm = (SQLMatchTrafficAlgorithm) TypedSPILoader.getService(TrafficAlgorithm.class, "SQL_MATCH",
Review Comment:
There is no need to assign to `sqlMatchAlgorithm`.
--
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] Netter99 commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Netter99 (via GitHub)" <gi...@apache.org>.
Netter99 commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501541304
> > @Pace2Car Please check again for my modification.
>
> @Netter99 Please add license comments for SegmentTrafficAlgorithmInitializationException.java
Hello, @Pace2Car ,what should I do to deal with the license. This is the first time I meet with this question. May you give me some advice?
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1161392624
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLMatch() {
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
assertFalse(sqlMatchAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order_item` SET `order_id` = ? WHERE user_id = ?;")));
}
+
+ @Test
+ void assertInitWithIllegalProps() {
+ assertThrows(IllegalArgumentException.class, () -> sqlMatchAlgorithm = TypedSPILoader.getService(SQLMatchTrafficAlgorithm.class, "SQL_MATCH",
Review Comment:
`sqlMatchAlgorithm =` is unnecessary and should be removed
##########
kernel/traffic/core/src/test/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLRegexTrafficAlgorithmTest.java:
##########
@@ -59,4 +60,10 @@ void assertMatchWhenNotExistSQLRegexMatch() {
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "TRUNCATE TABLE `t_order` ")));
assertFalse(sqlRegexAlgorithm.match(new SegmentTrafficValue(sqlStatement, "UPDATE `t_order` SET `order_id` = ?;")));
}
+
+ @Test
+ void assertInitWithIllegalProps() {
+ assertThrows(IllegalArgumentException.class, () -> sqlRegexAlgorithm = TypedSPILoader.getService(SQLRegexTrafficAlgorithm.class, "SQL_REGEX",
Review Comment:
`sqlMatchAlgorithm =` is unnecessary and should be removed
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1501746460
@Netter99 No need to close this PR, just wait for the CI result
--
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] Pace2Car commented on a diff in pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#discussion_r1160496433
##########
kernel/traffic/core/src/main/java/org/apache/shardingsphere/traffic/algorithm/traffic/segment/SQLMatchTrafficAlgorithm.java:
##########
@@ -43,6 +46,8 @@ public final class SQLMatchTrafficAlgorithm implements SegmentTrafficAlgorithm {
public void init(final Properties props) {
Preconditions.checkArgument(props.containsKey(SQL_PROPS_KEY), "%s cannot be null.", SQL_PROPS_KEY);
Review Comment:
Please check if the code was pushed successfully, I see no modification.
--
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] Pace2Car commented on pull request #25036: Improve properties verification of TrafficAlgorithm #24754
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25036:
URL: https://github.com/apache/shardingsphere/pull/25036#issuecomment-1502594488
@Netter99 Thanks for your contribution. Welcome to community!
--
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