abstractdog commented on code in PR #4683:
URL: https://github.com/apache/hive/pull/4683#discussion_r1390799216
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/YarnQueueHelper.java:
##########
@@ -101,8 +101,11 @@ public void checkQueueAccess(
checkQueueAccessInternal(queueName, userName);
return null;
});
- } catch (Exception exception) {
+ } catch (InterruptedException exception) {
+ // we need to catch only IO exception/Interrupted Exception here,
Review Comment:
the comment became outdated I believe:
```
we need to catch only IO exception/Interrupted
```
if we want to comment anything here, it should be inline with the current
situation, which is, in terms of exceptions:
- InterruptedException: thrown by ugi.doAs, we don't want to handle it here
- IOException: thrown by ugi.doAs, we don't want to handle it here
- HiveException: thrown by checkQueueAccessInternal, we must throw it
also, please remove this:
```
// as part of [HIVE-27029](https://issues.apache.org/jira/browse/HIVE-27029)
we are swallowing all exceptions and ignoring those causing yarn jobs are
submitted
```
code comments should not contain what we were doing wrong in the past, no
one cares about that :) a javadoc for the method is what we need, for example:
```
/**
* Checks yarn queue access of a given user for a given queue, and throws
* HiveException in case the user doesn't have access to the queue.
* @param queueName the yarn queue name
* @param userName the username
* @throws IOException when doAs throws an exception, we simply throw it
* @throws Interrupted when doAs throws an exception, we simply throw it
*/
```
regarding:
```
. now the user which does not have permission to access the queue, they cant
run the yarn jobs. these changes we cant verify with UT.
```
this is not necessarily true, I think you can still mock a YarnQueueHelper,
and check the following scenario:
- mock checkQueueAccessFromSingleRm to throw an IOException
- check if checkQueueAccess throws a HiveException
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
