sohami commented on a change in pull request #1626: DRILL-6855: Query from
non-existent proxy user fails with "No default schema selected" when
impersonation is enabled
URL: https://github.com/apache/drill/pull/1626#discussion_r251604318
##########
File path:
exec/java-exec/src/main/java/org/apache/calcite/jdbc/DynamicRootSchema.java
##########
@@ -115,8 +116,25 @@ public void loadSchemaFactory(String schemaName, boolean
caseSensitive) {
schemaPlus.add(wrapper.getName(), wrapper);
}
}
- } catch(ExecutionSetupException | IOException ex) {
+ } catch(ExecutionSetupException ex) {
logger.warn("Failed to load schema for \"" + schemaName + "\"!", ex);
+ } catch (IOException iex) {
+ // We can't proceed further without a schema, throw a runtime exception.
+ UserException.Builder exceptBuilder =
+ UserException
+ .resourceError(iex)
+ .message("Failed to create schema tree.")
+ .addContext("IOException: ", iex.getMessage());
+
+ // Improve the error message for client side.
+ final String errorMsg = "Error getting user info for current user";
+ if (iex.getMessage().startsWith(errorMsg)) {
+ final String contextString = "[Hint: Username is absent in connection
URL or doesn't " +
+ "exist on Drillbit node. Please
specify a username in connection " +
+ "URL which is present on Drillbit
node.]";
+ exceptBuilder.addContext(contextString);
+ }
+ throw exceptBuilder.build(logger);
Review comment:
I am not sure if there is any use case because of which the exceptions were
originally consumed and not thrown from here. If not and throwing exception is
fine then why not throw `UserException` for all the exceptions caught here ?
Also my preference will be that this errorMsg check and adding hint message
should be put as close to the original thrower of the IOException as possible.
Like in this case it will be
[here](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemSchemaFactory.java#L86).
Reason being then if we want to provide Hints for IOException from different
storage plugin then it can be done in each storage plugin rather than having
that logic here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
