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/03 15:46:35 UTC
[GitHub] [shardingsphere] GLBB opened a new pull request #9583: 9527
GLBB opened a new pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583
Fixes #ISSUSE_ID.
Changes proposed in this pull request:
-
- make exception message more detail
-
----------------------------------------------------------------
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
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583#discussion_r587994666
##########
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 (final IllegalArgumentException ex) {
+ throw new ShardingSphereConfigurationException("Incorrect configuration item: the property %s of the dataSource. the reason is %s", entry.getKey(), ex.getMessage());
Review comment:
Looks better. But at least it should be
> The reason is %s
Or maybe we can consider
> Incorrect configuration item: the property %s of the dataSource, because %s.
----------------------------------------------------------------
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
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [shardingsphere] tristaZero merged pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583
----------------------------------------------------------------
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
[GitHub] [shardingsphere] GLBB commented on a change in pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
GLBB commented on a change in pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583#discussion_r588304124
##########
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 (final IllegalArgumentException ex) {
+ throw new ShardingSphereConfigurationException("Incorrect configuration item: the property %s of the dataSource. the reason is %s", entry.getKey(), ex.getMessage());
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] GLBB commented on a change in pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
GLBB commented on a change in pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583#discussion_r587550679
##########
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:
how about this exception message: ยท`String.format("Incorrect configuration item: the property %s of the dataSource. the reason is %s", ex.getExceptionMessage())`
----------------------------------------------------------------
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
[GitHub] [shardingsphere] GLBB commented on a change in pull request #9583: #9527 make exception message more detail
Posted by GitBox <gi...@apache.org>.
GLBB commented on a change in pull request #9583:
URL: https://github.com/apache/shardingsphere/pull/9583#discussion_r587539361
##########
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:
yes, can replace. in the first place, I thought origin exception stack can see datasource which setter method error, but after try, it can't see which method error
----------------------------------------------------------------
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