Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed ThreadConfigurableLogger class for upcoming SLF4J change #2615

Open
wants to merge 1 commit into
base: integration
Choose a base branch
from

Conversation

doublejai
Copy link
Collaborator

No description provided.

@@ -544,7 +544,7 @@ public ScannerStream visit(ASTEQNode node, Object data) {
return ScannerStream.unindexed(node);
}
} catch (TableNotFoundException e) {
log.error(e);
log.error(String.valueOf(e));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should just be log.error(e). Is there no method for that? We want to log the full stacktrace. If we convert to a String, then we lose that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So we need some kind of message. We might be able to get away with log.error("", e);.

@@ -122,7 +122,7 @@ public synchronized void submit() {
range = new Range(startKey, true, endKey, literalRange.isUpperInclusive());
} catch (IllegalArgumentException e) {
QueryException qe = new QueryException(DatawaveErrorCode.RANGE_CREATE_ERROR, e, MessageFormat.format("{0}", this.literalRange));
log.debug(qe);
log.debug(String.valueOf(qe));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here. We need the exception treated like a Throwable in order to see the stacktrace. Let's try log.debug("", qe);

@@ -91,7 +91,7 @@ public static <T extends JexlNode> T expandUnfielded(ShardQueryConfiguration con
private static <T extends JexlNode> T ensureTreeNotEmpty(T script) throws EmptyUnfieldedTermExpansionException {
if (script.jjtGetNumChildren() == 0) {
NotFoundQueryException qe = new NotFoundQueryException(DatawaveErrorCode.NO_UNFIELDED_TERM_EXPANSION_MATCH);
log.warn(qe);
log.warn(String.valueOf(qe));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here.

@@ -1416,11 +1416,11 @@ protected ASTJexlScript timedExpandAnyFieldRegexNodes(QueryStopwatch timers, fin
} catch (EmptyUnfieldedTermExpansionException e) {
// The visitor will only throw this if we cannot expand anything resulting in empty query
NotFoundQueryException qe = new NotFoundQueryException(DatawaveErrorCode.UNFIELDED_QUERY_ZERO_MATCHES, e, MessageFormat.format("Query: ", query));
log.info(qe);
log.info(String.valueOf(qe));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna stop commenting on these but just do a search for any String.valueOf and use the empty message pattern mentioned above.

@@ -439,8 +437,8 @@ public void updateMetric(QueryMetric updatedQueryMetric, DatawavePrincipal dataw

// write new entry
writeMetrics(updatedQueryMetric, Collections.singletonList(updatedQueryMetric), lastUpdated, false);
} finally {
enableLogs(true);
} catch (Exception e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can't do an empty catch here b/c that means we suppress an error. To keep this functionality the same as before, we should drop the try around the code block above.

try {
  // some code
} finally {
}

Is equivalent to

// some code

@@ -550,8 +547,8 @@ public QueryMetricListResponse query(String user, String queryId, DatawavePrinci
response.setResult(queryMetrics);

response.setGeoQuery(queryMetrics.stream().anyMatch(SimpleQueryGeometryHandler::isGeoQuery));
} finally {
enableLogs(true);
} catch (Exception e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants