CODE: pass over looking at exception handling and reporting [KQL-395] #395
Conversation
bluemonk3y
changed the title
| @@ -155,7 +154,7 @@ public static boolean createFile(Path path) { | |||
| } | |||
| return true; | |||
| } catch (Exception e) { | |||
| LOGGER.error(ExceptionUtil.stackTraceToString(e)); | |||
| LOGGER.error("createFile failed, path:" + path, e); | |||
There was a problem hiding this comment.
is this an error or should it be warn?
There was a problem hiding this comment.
also here and everywhere else in the log statements we should use "{}" for args rather than string concatentation
| @@ -159,11 +159,9 @@ private Schema getKsqlType(final String sqlType) { | |||
|
|
|||
| private Schema getKsqlComplexType(final String sqlType) { | |||
| if (sqlType.startsWith("ARRAY")) { | |||
| return SchemaBuilder | |||
| .array(getKsqlType(sqlType.substring("ARRAY".length() + 1, sqlType.length() - 1))); | |||
| return SchemaBuilder.array(getKsqlType(sqlType.substring("ARRAY".length() + 1, sqlType.length() - 1))); | |||
There was a problem hiding this comment.
perhaps we should turn "ARRAY" and "MAP" into constants?
| } else if (sqlType.startsWith("MAP")) { | ||
| //TODO: For now only primitive data types for map are supported. Will have to add | ||
| // nested types. | ||
| //TODO: For now only primitive data types for map are supported. Will have to add nested types. |
| @@ -61,7 +61,7 @@ private static DDLCommandResult executeOnMetaStore(DDLCommand ddlCommand, MetaSt | |||
| return ddlCommand.run(metaStore); | |||
| } catch (Exception e) { | |||
| String stackTrace = ExceptionUtil.stackTraceToString(e); | |||
| LOGGER.error(stackTrace); | |||
| LOGGER.error("executeOnMetaStore:" + ddlCommand + " failed", e); | |||
There was a problem hiding this comment.
should this be warn? I see error as something that is going to blow everything up, but this continues.
| @@ -301,7 +297,7 @@ private SourceDescription describe(String statementText, String name) throws Exc | |||
|
|
|||
| StructuredDataSource dataSource = ksqlEngine.getMetaStore().getSource(name); | |||
| if (dataSource == null) { | |||
| throw new Exception(String.format("Could not find data stream/table '%s' in the metastore", | |||
| throw new Exception(String.format("Could not find STREAM/TABLE '%s' in the Metastore", | |||
There was a problem hiding this comment.
I know this was here already, but throwing Exception - really? above too
There was a problem hiding this comment.
i think it propagates to the client where it gets logged
…ious fixes as per the PR
|
retest this please |
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
@dguy - take a look see pls |
|
retest this please |
There was a problem hiding this comment.
Thanks @bluemonk3y, you don't need the %s in the logging statements, just {}
| @@ -523,7 +523,7 @@ private void printAsJson(Object o) throws IOException { | |||
| } | |||
| o = newEntities; | |||
| } else { | |||
| LOGGER.warn(String.format("Unexpected result class: '%s' found in printAsJson", o.getClass().getCanonicalName())); | |||
| log.warn("Unexpected result class: '{%s}' found in printAsJson", o.getClass().getCanonicalName()); | |||
| @@ -155,7 +154,7 @@ public static boolean createFile(Path path) { | |||
| } | |||
| return true; | |||
| } catch (Exception e) { | |||
| LOGGER.error(ExceptionUtil.stackTraceToString(e)); | |||
| log.warn("createFile failed, path: {%s}", path, e); | |||
There was a problem hiding this comment.
same here and everywhere else i guess
#394 - pass over to review exception and log handling.