You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "liuxiaocs7 (via GitHub)" <gi...@apache.org> on 2023/04/19 18:34:02 UTC
[GitHub] [shardingsphere] liuxiaocs7 opened a new pull request, #25246: Improve properties verification of ClassBasedShardingAlgorithm
liuxiaocs7 opened a new pull request, #25246:
URL: https://github.com/apache/shardingsphere/pull/25246
- SubTask of #24756
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.
- [X] 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] Pace2Car commented on a diff in pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1176060078
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithm.java:
##########
@@ -59,15 +60,18 @@ public void init(final Properties props) {
private ClassBasedShardingAlgorithmStrategyType getStrategy(final Properties props) {
String strategy = props.getProperty(STRATEGY_KEY);
- ShardingSpherePreconditions.checkNotNull(strategy,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", STRATEGY_KEY)));
- return ClassBasedShardingAlgorithmStrategyType.valueOf(strategy.toUpperCase().trim());
+ ShardingSpherePreconditions.checkState(!Strings.isNullOrEmpty(strategy),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null or empty when uses class based sharding strategy", STRATEGY_KEY)));
+ String shardingAlgorithmStrategyType = strategy.toUpperCase().trim();
+ ShardingSpherePreconditions.checkState(
+ ClassBasedShardingAlgorithmStrategyType.isValidShardingAlgorithmStrategyType(shardingAlgorithmStrategyType),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Sharding strategy `%s` not support", strategy)));
+ return ClassBasedShardingAlgorithmStrategyType.valueOf(shardingAlgorithmStrategyType);
}
private String getAlgorithmClassName(final Properties props) {
String result = props.getProperty(ALGORITHM_CLASS_NAME_KEY);
- ShardingSpherePreconditions.checkNotNull(result,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", ALGORITHM_CLASS_NAME_KEY)));
+ ShardingSpherePreconditions.checkState(!Strings.isNullOrEmpty(result), () -> new ShardingAlgorithmInitializationException(getType(), "Sharding algorithm ClassName can not be null or empty"));
Review Comment:
please replace `ClassName` to `class 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] liuxiaocs7 commented on pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#issuecomment-1520089741
Hi, @Pace2Car, I have fixed all comments and formatted code use `mvn spotless:apply`, PTAL again, thanks!
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#issuecomment-1524603599
Hi @liuxiaocs7, thanks for your contribution in this issue, the verification of `ClassBasedShardingAlgorithm` seems to be ok, please refer to this style to complete other subtasks.
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174529275
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithm.java:
##########
@@ -60,14 +60,19 @@ public void init(final Properties props) {
private ClassBasedShardingAlgorithmStrategyType getStrategy(final Properties props) {
String strategy = props.getProperty(STRATEGY_KEY);
ShardingSpherePreconditions.checkNotNull(strategy,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", STRATEGY_KEY)));
- return ClassBasedShardingAlgorithmStrategyType.valueOf(strategy.toUpperCase().trim());
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy", STRATEGY_KEY)));
+ String shardingAlgorithmStrategyType = strategy.toUpperCase().trim();
+ ShardingSpherePreconditions.checkState(
+ ClassBasedShardingAlgorithmStrategyType.isValidShardingAlgorithmStrategyType(shardingAlgorithmStrategyType),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Sharding strategy `%s` not support", strategy)));
+ return ClassBasedShardingAlgorithmStrategyType.valueOf(shardingAlgorithmStrategyType);
}
-
+
Review Comment:
Please keep the same indentation with the previous line.
--
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] liuxiaocs7 commented on a diff in pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1175228068
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithmStrategyType.java:
##########
@@ -25,15 +25,25 @@ public enum ClassBasedShardingAlgorithmStrategyType {
/**
* The sharding strategy is standard.
*/
- STANDARD,
+ STANDARD("STANDARD"),
/**
* The sharding strategy is complex.
*/
- COMPLEX,
+ COMPLEX("COMPLEX"),
/**
* The sharding strategy is hint.
*/
- HINT
+ HINT("HINT");
+
+ private final String type;
+
+ ClassBasedShardingAlgorithmStrategyType(final String type) {
+ this.type = type;
+ }
+
+ static boolean isValidShardingAlgorithmStrategyType(final String type) {
+ return STANDARD.type.equalsIgnoreCase(type) || COMPLEX.type.equalsIgnoreCase(type) || HINT.type.equalsIgnoreCase(type);
Review Comment:
> No need to define `type` field, use
>
> ```java
> static boolean isValidShardingAlgorithmStrategyType(final String type) {
> return Arrays.stream(values()).anyMatch(each -> each.name().equals(type));
> }
> ```
got, thanks!
--
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] liuxiaocs7 commented on a diff in pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1176362786
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithm.java:
##########
@@ -59,15 +60,18 @@ public void init(final Properties props) {
private ClassBasedShardingAlgorithmStrategyType getStrategy(final Properties props) {
String strategy = props.getProperty(STRATEGY_KEY);
- ShardingSpherePreconditions.checkNotNull(strategy,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", STRATEGY_KEY)));
- return ClassBasedShardingAlgorithmStrategyType.valueOf(strategy.toUpperCase().trim());
+ ShardingSpherePreconditions.checkState(!Strings.isNullOrEmpty(strategy),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null or empty when uses class based sharding strategy", STRATEGY_KEY)));
+ String shardingAlgorithmStrategyType = strategy.toUpperCase().trim();
+ ShardingSpherePreconditions.checkState(
+ ClassBasedShardingAlgorithmStrategyType.isValidShardingAlgorithmStrategyType(shardingAlgorithmStrategyType),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Sharding strategy `%s` not support", strategy)));
+ return ClassBasedShardingAlgorithmStrategyType.valueOf(shardingAlgorithmStrategyType);
}
private String getAlgorithmClassName(final Properties props) {
String result = props.getProperty(ALGORITHM_CLASS_NAME_KEY);
- ShardingSpherePreconditions.checkNotNull(result,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", ALGORITHM_CLASS_NAME_KEY)));
+ ShardingSpherePreconditions.checkState(!Strings.isNullOrEmpty(result), () -> new ShardingAlgorithmInitializationException(getType(), "Sharding algorithm ClassName can not be null or empty"));
Review Comment:
fixed.
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174529306
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithmStrategyType.java:
##########
@@ -25,15 +25,25 @@ public enum ClassBasedShardingAlgorithmStrategyType {
/**
* The sharding strategy is standard.
*/
- STANDARD,
+ STANDARD("STANDARD"),
/**
* The sharding strategy is complex.
*/
- COMPLEX,
+ COMPLEX("COMPLEX"),
/**
* The sharding strategy is hint.
*/
- HINT
+ HINT("HINT");
+
+ private final String type;
+
+ ClassBasedShardingAlgorithmStrategyType(final String type) {
+ this.type = type;
+ }
+
Review Comment:
indentation here.
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174528654
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithm.java:
##########
@@ -60,14 +60,19 @@ public void init(final Properties props) {
private ClassBasedShardingAlgorithmStrategyType getStrategy(final Properties props) {
String strategy = props.getProperty(STRATEGY_KEY);
ShardingSpherePreconditions.checkNotNull(strategy,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", STRATEGY_KEY)));
- return ClassBasedShardingAlgorithmStrategyType.valueOf(strategy.toUpperCase().trim());
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy", STRATEGY_KEY)));
+ String shardingAlgorithmStrategyType = strategy.toUpperCase().trim();
+ ShardingSpherePreconditions.checkState(
+ ClassBasedShardingAlgorithmStrategyType.isValidShardingAlgorithmStrategyType(shardingAlgorithmStrategyType),
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Sharding strategy `%s` not support", strategy)));
+ return ClassBasedShardingAlgorithmStrategyType.valueOf(shardingAlgorithmStrategyType);
}
-
+
private String getAlgorithmClassName(final Properties props) {
String result = props.getProperty(ALGORITHM_CLASS_NAME_KEY);
ShardingSpherePreconditions.checkNotNull(result,
- () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy.", ALGORITHM_CLASS_NAME_KEY)));
+ () -> new ShardingAlgorithmInitializationException(getType(), String.format("Properties `%s` can not be null when uses class based sharding strategy", ALGORITHM_CLASS_NAME_KEY)));
+ ShardingSpherePreconditions.checkState(!"".equals(result), () -> new ShardingAlgorithmInitializationException(getType(), "Sharding algorithm ClassName can not be a empty string"));
Review Comment:
These two checks can be combined into `!Strings.isNullOrEmpty(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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174528178
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithmStrategyType.java:
##########
@@ -25,15 +25,25 @@ public enum ClassBasedShardingAlgorithmStrategyType {
/**
* The sharding strategy is standard.
*/
- STANDARD,
+ STANDARD("STANDARD"),
/**
* The sharding strategy is complex.
*/
- COMPLEX,
+ COMPLEX("COMPLEX"),
/**
* The sharding strategy is hint.
*/
- HINT
+ HINT("HINT");
+
+ private final String type;
+
+ ClassBasedShardingAlgorithmStrategyType(final String type) {
+ this.type = type;
+ }
+
+ static boolean isValidShardingAlgorithmStrategyType(final String type) {
+ return STANDARD.type.equalsIgnoreCase(type) || COMPLEX.type.equalsIgnoreCase(type) || HINT.type.equalsIgnoreCase(type);
Review Comment:
No need to define `type` field, use
```java
static boolean isValidShardingAlgorithmStrategyType(final String type) {
return Arrays.stream(values()).anyMatch(each -> each.name().equals(type));
}
```
--
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] liuxiaocs7 closed pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 closed pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
URL: https://github.com/apache/shardingsphere/pull/25246
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174529353
##########
features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithmTest.java:
##########
@@ -50,14 +50,22 @@ void assertInitWithNullStrategy() {
@Test
void assertInitWithWrongStrategy() {
- assertThrows(IllegalArgumentException.class, () -> TypedSPILoader.getService(ShardingAlgorithm.class, "CLASS_BASED", PropertiesBuilder.build(new Property("strategy", "wrong"))));
+ assertThrows(ShardingAlgorithmInitializationException.class,
+ () -> TypedSPILoader.getService(ShardingAlgorithm.class, "CLASS_BASED", PropertiesBuilder.build(new Property("strategy", "wrong"))));
}
@Test
void assertInitWithNullClass() {
assertThrows(ShardingAlgorithmInitializationException.class,
() -> TypedSPILoader.getService(ShardingAlgorithm.class, "CLASS_BASED", PropertiesBuilder.build(new Property("strategy", "standard"))));
}
+
Review Comment:
indentation.
--
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 #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "Pace2Car (via GitHub)" <gi...@apache.org>.
Pace2Car commented on code in PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#discussion_r1174528847
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/algorithm/sharding/classbased/ClassBasedShardingAlgorithm.java:
##########
@@ -60,14 +60,19 @@ public void init(final Properties props) {
private ClassBasedShardingAlgorithmStrategyType getStrategy(final Properties props) {
String strategy = props.getProperty(STRATEGY_KEY);
ShardingSpherePreconditions.checkNotNull(strategy,
Review Comment:
This can also be enhanced to `!Strings.isNullOrEmpty(strategy)`
--
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] liuxiaocs7 commented on pull request #25246: Improve properties verification of ClassBasedShardingAlgorithm
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #25246:
URL: https://github.com/apache/shardingsphere/pull/25246#issuecomment-1536599906
> Hi @liuxiaocs7, thanks for your contribution in this issue, the verification of `ClassBasedShardingAlgorithm` seems to be ok, please refer to this style to complete other subtasks.
Hi, @Pace2Car, sorry for late, got it, thanks, I'll continue to work on others algorithms based on this example!
--
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