-
Notifications
You must be signed in to change notification settings - Fork 91
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
Also interpret cost JSON object in case it is returned by oak search engine #731
Also interpret cost JSON object in case it is returned by oak search engine #731
Conversation
|
||
// use jackson for JSON parsing | ||
ObjectMapper mapper = new ObjectMapper(); | ||
String costJsonStr = plan.substring(plan.lastIndexOf('{')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should log the JSON (on debug/verbose level) in case it changes in the future, so that we can more easily come up with a fix.
// read the json strings and convert it into JsonNode | ||
JsonNode node = mapper.readTree(jsonStr); | ||
|
||
JsonNode sNode = node.get("s"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some more null checks here and a unit test with invalid JSON.
JsonNode node = mapper.readTree(jsonStr); | ||
|
||
JsonNode sNode = node.get("s"); | ||
double cost = sNode.isContainerNode() ? sNode.get("perExecution").asDouble(Double.MAX_VALUE) : sNode.asDouble(Double.MAX_VALUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let us null check sNode.get("perExecution") as well.
Include index usage in AC Tool log Fix JSON retrieval from plan
9f855a5
to
8456ecf
Compare
} catch(IOException|RepositoryException e) { | ||
LOG.warn("Cannot figure out if query index for rep:ACL nodes exist", e); | ||
} catch(Exception e) { | ||
LOG.warn("Could not detect if query index for rep:ACL nodes exists: {}", e.getMessage(), e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no matter what exception is thrown this warning should be shown
// use jackson for JSON parsing | ||
ObjectMapper mapper = new ObjectMapper(); | ||
LOG.debug("Execution plan and cost for {}: {}", EXPLAIN_QUERY_FOR_ACL_INDEX, plan); | ||
String costJsonStr = StringUtils.substringAfter(plan, "cost:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking for {
is also very unsafe - conceptually better to check for the string after cost:
. (plan.substring(plan.lastIndexOf('{'))
returned only parts of the cost json)
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes, looks good now.
Closes #730