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 2021/03/04 06:03:59 UTC
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #9583: #9527 make exception message more detail
tristaZero commented on a change in pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583#discussion_r587168946
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/config/datasource/DataSourceConfiguration.java
##########
@@ -111,9 +112,13 @@ public DataSource createDataSource() {
if (SKIPPED_PROPERTY_NAMES.contains(entry.getKey())) {
continue;
}
- Optional<Method> setterMethod = findSetterMethod(methods, entry.getKey());
- if (setterMethod.isPresent()) {
- setterMethod.get().invoke(result, entry.getValue());
+ try {
+ Optional<Method> setterMethod = findSetterMethod(methods, entry.getKey());
+ if (setterMethod.isPresent()) {
+ setterMethod.get().invoke(result, entry.getValue());
+ }
+ } catch (Exception ex) {
+ throw new ShardingSphereConfigurationException(ex, String.format("datasource property %s configure error", entry.getKey()));
Review comment:
The error info needs more consideration since that will be exposed to users.
How about `Incorrect configuration item: the property %s of the dataSource`?
That's just my draft. It's up to you, however please be careful of English grammar.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/config/datasource/DataSourceConfiguration.java
##########
@@ -111,9 +112,13 @@ public DataSource createDataSource() {
if (SKIPPED_PROPERTY_NAMES.contains(entry.getKey())) {
continue;
}
- Optional<Method> setterMethod = findSetterMethod(methods, entry.getKey());
- if (setterMethod.isPresent()) {
- setterMethod.get().invoke(result, entry.getValue());
+ try {
+ Optional<Method> setterMethod = findSetterMethod(methods, entry.getKey());
+ if (setterMethod.isPresent()) {
+ setterMethod.get().invoke(result, entry.getValue());
+ }
+ } catch (Exception ex) {
Review comment:
Please define it as `final Exception ex`
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/config/exception/ShardingSphereConfigurationException.java
##########
@@ -42,4 +42,14 @@ public ShardingSphereConfigurationException(final String errorMessage, final Obj
public ShardingSphereConfigurationException(final Exception cause) {
super(cause);
}
+
+ /**
+ * Constructs an exception with cause exception and error message.
+ *
+ * @param cause cause exception
+ * @param errorMessage error message
+ */
+ public ShardingSphereConfigurationException(final Exception cause, final String errorMessage) {
Review comment:
Do you think `public ShardingSphereConfigurationException(final String errorMessage, final Object... args)` of this class can replace this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org