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/05 13:16:47 UTC
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #22677: Fix #22658 to make sure ShardingSphereDriver can load configuration file
sandynz commented on code in PR #22677:
URL: https://github.com/apache/shardingsphere/pull/22677#discussion_r1039577568
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
+
+ for (ClassLoader cl : classLoader) {
Review Comment:
Variable name `cl` should be `each`
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
+
+ for (ClassLoader cl : classLoader) {
+ if (null != cl) {
+
+ // try to find the resource as passed
+ InputStream returnValue = cl.getResourceAsStream(resource);
+
+ // now, some class loaders want this leading "/", so we'll add it and try again if we didn't find the resource
+ if (null == returnValue) {
+ returnValue = cl.getResourceAsStream("/" + resource);
+ }
Review Comment:
Did you test which class loader need '/'?
If the most frequent used one need '/', could we try to load with '/' firstly?
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
+
+ for (ClassLoader cl : classLoader) {
+ if (null != cl) {
+
+ // try to find the resource as passed
+ InputStream returnValue = cl.getResourceAsStream(resource);
+
+ // now, some class loaders want this leading "/", so we'll add it and try again if we didn't find the resource
+ if (null == returnValue) {
+ returnValue = cl.getResourceAsStream("/" + resource);
+ }
+
Review Comment:
Empty lines in method could be removed
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
Review Comment:
`classLoader` could be `classLoaders`
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
Review Comment:
Did you test which class loader satisify most cases or the most common case?
Could we put `getClass().getClassLoader()` at first?
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
+
+ for (ClassLoader cl : classLoader) {
+ if (null != cl) {
+
+ // try to find the resource as passed
+ InputStream returnValue = cl.getResourceAsStream(resource);
Review Comment:
`returnValue` should be `result`
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
+
+ for (ClassLoader cl : classLoader) {
+ if (null != cl) {
+
+ // try to find the resource as passed
Review Comment:
Comments in method could be removed
##########
jdbc/core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/driver/ShardingSphereDriverURL.java:
##########
@@ -72,4 +72,30 @@ public byte[] toConfigurationBytes() {
return builder.toString().getBytes(StandardCharsets.UTF_8);
}
}
+
+ private InputStream getResourceAsStream(final String resource) throws IOException {
+ ClassLoader[] classLoader = new ClassLoader[]{
+ Thread.currentThread().getContextClassLoader(),
+ getClass().getClassLoader(),
+ ClassLoader.getSystemClassLoader(),
+ };
Review Comment:
It's better to put them in one line for shorter code
--
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