ctubbsii commented on code in PR #6423:
URL: https://github.com/apache/accumulo/pull/6423#discussion_r3415452702
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:
##########
@@ -1470,12 +1476,17 @@ public TExternalCompactionJob
reserveCompactionJob(TInfo tinfo, TCredentials cre
@Override
public void compactionJobFinished(TInfo tinfo, TCredentials credentials,
- String externalCompactionId, TKeyExtent extent, long fileSize, long
entries)
- throws ThriftSecurityException, TException {
+ String externalCompactionId, TKeyExtent extent, long fileSize, long
entries) {
- if (!security.canPerformSystemActions(credentials)) {
- throw new AccumuloSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED).asThriftException();
+ try {
+ if (!security.canPerformSystemActions(credentials)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
+ }
+ } catch (ThriftSecurityException e) {
+ log.warn(
+ "Received compaction job finished message (id: {}) from user ({})
with bad credentials",
+ externalCompactionId, credentials.getPrincipal());
}
Review Comment:
You're right about the missing return. I had thought this was the end of the
method. But, we still need to handle both the exception from
security.canPerformSystemActions as well as its return value. I chose to throw
an exception and handle it in one place, but another option is to log it and
return in two places.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
