You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/05/16 15:04:12 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3402: Improve some exception handling
ctubbsii opened a new pull request, #3402:
URL: https://github.com/apache/accumulo/pull/3402
Use a more specific RuntimeException type, for a large number of appropriate situations. Clean up some unnecessary wrapping of RuntimeExceptions with a new RuntimeException.
Also, try to catch more specific checked exceptions, when appropriate, instead of catching (Exception) and do the same in non-public API methods where Exception was being thrown, forcing catch blocks to handle a wider number of exceptions than were actually thrown.
This change represents an incremental improvement, and does not try to be an exhaustive fix of all these similar issues. There may be other opportunities for similar improvements that are not included in this change and can be done in a subsequent change. Rather, this more narrowly fixes the occurrences originally included as part of abandoned PR #2762 to address spotbugs warnings that no longer occur on the latest version of spotbugs.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196811749
##########
core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java:
##########
@@ -163,7 +163,7 @@ interface Configuration extends Iterable<Entry<String,String>> {
* @param className Fully qualified name of the class.
* @param base The expected super type of the class.
*/
- <T> T instantiate(String className, Class<T> base) throws Exception;
+ <T> T instantiate(String className, Class<T> base) throws ReflectiveOperationException;
Review Comment:
Not sure. Seems like some source could have been trying to catch stuff not possible to be thrown, and compiler wouldn't have cared, but now with the narrower exception, compiler will probably flag those as an attempt to catch something not thrown. Not sure about binary, but not expecting binary compatibility across major versions anyway.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196823119
##########
core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java:
##########
@@ -398,7 +398,7 @@ private static void attemptClientReLogin() {
// The inability to check is worrisome and deserves a RuntimeException instead of a propagated
// IO-like Exception.
log.warn("Failed to check (and/or perform) Kerberos client re-login", e);
- throw new RuntimeException(e);
+ throw new UncheckedIOException(e);
Review Comment:
Originally committed this, but reverted because this (or another one) broke the formatter, and my change is more minimal. This might be an improvement, but not one I'm inclined to bother with here. Maybe it'd be worth putting the message in a String variable and reusing it, but that's out of scope of my change, which is just using the more specific type.
##########
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java:
##########
@@ -151,7 +151,7 @@ public int getTermEnd() {
public ByteSequence getTerm(byte[] expression) {
if (type != NodeType.TERM) {
- throw new RuntimeException();
+ throw new IllegalStateException();
Review Comment:
Originally committed this, but reverted because this (or another one) broke the formatter, and my change is more minimal. This might be an improvement, but not one I'm inclined to bother with here. Maybe it'd be worth putting the message in a String variable and reusing it, but that's out of scope of my change, which is just using the more specific 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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195554297
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
This may be easier to follow if the returns were kept, but the else clauses removed. (more of a personnel preference?)
`if(string){
...
return
}
if(List) {
...
return
}
throw new IllegalStateException`
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196806921
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
I responded too quickly before a meeting I was going to. I'll take another look.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3402:
URL: https://github.com/apache/accumulo/pull/3402
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195559613
##########
core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java:
##########
@@ -398,7 +398,7 @@ private static void attemptClientReLogin() {
// The inability to check is worrisome and deserves a RuntimeException instead of a propagated
// IO-like Exception.
log.warn("Failed to check (and/or perform) Kerberos client re-login", e);
- throw new RuntimeException(e);
+ throw new UncheckedIOException(e);
Review Comment:
```suggestion
throw new UncheckedIOException("Failed to check (and/or perform) Kerberos client re-login", e);
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196747214
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
The nesting of the else clauses made it more difficult to examine your changes to see that the behavior did not change.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195557682
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java:
##########
@@ -118,9 +118,9 @@ public ServiceLock(ZooKeeper zookeeper, ServiceLockPath path, UUID uuid) {
zooKeeper.exists(path.toString(), this);
watchingParent = true;
this.vmLockPrefix = new Prefix(ZLOCK_PREFIX + uuid.toString() + "#");
- } catch (Exception ex) {
+ } catch (KeeperException | InterruptedException ex) {
LOG.error("Error setting initial watch", ex);
- throw new RuntimeException(ex);
+ throw new IllegalStateException(ex);
Review Comment:
```suggestion
throw new IllegalStateException("Error setting initial watch", ex);
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196819154
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
I took another look, and I think it's fine the way it is. It seems like a slight trade-off, mostly style preferences. I didn't write this code originally, and don't have a stake in one style or the other... but I'm inclined to keep the more minimal change in this PR.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196822935
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLock.java:
##########
@@ -118,9 +118,9 @@ public ServiceLock(ZooKeeper zookeeper, ServiceLockPath path, UUID uuid) {
zooKeeper.exists(path.toString(), this);
watchingParent = true;
this.vmLockPrefix = new Prefix(ZLOCK_PREFIX + uuid.toString() + "#");
- } catch (Exception ex) {
+ } catch (KeeperException | InterruptedException ex) {
LOG.error("Error setting initial watch", ex);
- throw new RuntimeException(ex);
+ throw new IllegalStateException(ex);
Review Comment:
Originally committed this, but reverted because this (or another one) broke the formatter, and my change is more minimal. This might be an improvement, but not one I'm inclined to bother with here. Maybe it'd be worth putting the message in a String variable and reusing it, but that's out of scope of my change, which is just using the more specific 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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195561271
##########
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java:
##########
@@ -151,7 +151,7 @@ public int getTermEnd() {
public ByteSequence getTerm(byte[] expression) {
if (type != NodeType.TERM) {
- throw new RuntimeException();
+ throw new IllegalStateException();
Review Comment:
```suggestion
throw new IllegalStateException("unexpected node 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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3402: Improve some exception handling
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195688330
##########
core/src/main/java/org/apache/accumulo/core/client/PluginEnvironment.java:
##########
@@ -163,7 +163,7 @@ interface Configuration extends Iterable<Entry<String,String>> {
* @param className Fully qualified name of the class.
* @param base The expected super type of the class.
*/
- <T> T instantiate(String className, Class<T> base) throws Exception;
+ <T> T instantiate(String className, Class<T> base) throws ReflectiveOperationException;
Review Comment:
Will this change cause any compatibility issues? Thinking it will not, but not completely sure. Seems like its source compatible, not sure about binary.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3402: Improve some exception handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1195554297
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
This may be easier to follow if the returns were kept, but the else clauses removed. (more of a personnel preference?)
```
if(string){
...
return
}
if(List) {
...
return
}
throw new IllegalStateException
```
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
This may be easier to follow if the returns were kept, but the else clauses removed. (more of a personnel preference?)
```java
if(string){
...
return
}
if(List) {
...
return
}
throw new IllegalStateException
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3402: Improve some exception handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3402:
URL: https://github.com/apache/accumulo/pull/3402#discussion_r1196663217
##########
core/src/main/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParser.java:
##########
@@ -61,14 +61,12 @@ private static void flatten(String parentKey, String key, Object value,
(parentKey == null || parentKey.equals("")) ? "" : parentKey + addTheDot(parentKey);
if (value instanceof String) {
results.put(parent + key, (String) value);
- return;
Review Comment:
The method is already very short. These aren't needed.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org