Add more logs for UnableToPlanException #3766
Conversation
pengpeng-lu
added
planner
| * @return this exception for chaining | ||
| */ | ||
| @Nonnull | ||
| public UnableToPlanException withPlanCacheInfo(@Nullable String planCacheInfo) { |
There was a problem hiding this comment.
This class should not have anything in it referring to upper layers. The match candidates are fine and needed.
There was a problem hiding this comment.
removed planCacheInfo from UnableToPlanException, log it directly in PlanGenerator.
There was a problem hiding this comment.
and also removed connectionOptionsInfo and log it directly in PlanGenerator.
| */ | ||
| @Nonnull | ||
| public UnableToPlanException withPlanCacheInfo(@Nullable String planCacheInfo) { | ||
| this.planCacheInfo = planCacheInfo; |
There was a problem hiding this comment.
Let's not have this mutable.
| matchCandidatesBuilder.append(" (none)"); | ||
| } else { | ||
| for (final var candidate : matchCandidates) { | ||
| matchCandidatesBuilder.append(String.format(" - %s%n", candidate.toString())); |
There was a problem hiding this comment.
Are we fine with just logging the kind of index and the index name here? candidate.toString() in ValueIndexScanMatchCandidate is implemented in a way that would print:
value[someIndexName]
Should we rather just add the MatchCandidates themselves here? In that way the consumer can decide what to do with them.
There was a problem hiding this comment.
what functions of MatchCandidate shows the complete graph? I added some more info in here, but not sure if that is enough.
| } catch (UnableToPlanException e) { | ||
| if (logger.isErrorEnabled()) { | ||
| final var logBuilder = KeyValueLogMessage.build("Unable to plan query", | ||
| "recordMetadata", metaData.toString(), |
There was a problem hiding this comment.
This one does not have a toString() unless I missed that.
There was a problem hiding this comment.
replace it with toProto().toString().
| planContext.getConstantActionFactory(), planContext.getDbUri(), caseSensitive) | ||
| .generateLogicalPlan(ast.getParseTree())); | ||
| return maybePlan.optimize(planner, planContext, currentPlanHashMode); | ||
| } catch (UnableToPlanException e) { |
There was a problem hiding this comment.
Can you please add a test for this? we should test cases where:
- we do not have cache.
- we have cache but cold.
- we have cache that is warm.
| // Attach connection options information to the exception | ||
| final var optionsBuilder = new StringBuilder("Connection Options:\n"); | ||
| for (final var entry : options.entries()) { | ||
| optionsBuilder.append(String.format(" %s = %s%n", entry.getKey(), entry.getValue())); |
There was a problem hiding this comment.
If I am not mistaken, some values can be null, is that ok for the StringBuilder?
There was a problem hiding this comment.
good catch, log string null when it is null.
| @@ -0,0 +1,106 @@ | |||
| /* | |||
| * UnableToPlanExceptionTest.java | |||
There was a problem hiding this comment.
These tests may be useful, but they seem a bit superficial to me, can we add integration tests where we verify the exception's behavior in a real scenario where the planner fails to plan? please see my other comment.
There was a problem hiding this comment.
yes this test was actually added to meet TeamScale requirement, added a test around plan cache.
3 participants
No description provided.