-
Notifications
You must be signed in to change notification settings - Fork 117
Add more logs for UnableToPlanException #3766
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
base: main
Are you sure you want to change the base?
Changes from all commits
0f045b9
beabd0d
2fe31b4
2c7e9e7
b8ba57d
3763214
448b578
f342eaf
e74b80d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * UnableToPlanExceptionTest.java | ||
| * | ||
| * This source file is part of the FoundationDB open source project | ||
| * | ||
| * Copyright 2025 Apple Inc. and the FoundationDB project authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.apple.foundationdb.record.query.plan.cascades; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertSame; | ||
|
|
||
| /** | ||
| * Tests for {@link UnableToPlanException}. | ||
| */ | ||
| public class UnableToPlanExceptionTest { | ||
| @Test | ||
| public void testWithMatchCandidatesInfo() { | ||
|
Check warning on line 34 in fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/UnableToPlanExceptionTest.java
|
||
| final UnableToPlanException exception = new UnableToPlanException("Test message"); | ||
| final String matchCandidatesInfo = "Match Candidates:\n - candidate1\n - candidate2"; | ||
|
|
||
| final UnableToPlanException result = exception.withMatchCandidatesInfo(matchCandidatesInfo); | ||
|
|
||
| // Verify it returns the same instance (fluent API) | ||
| assertSame(exception, result); | ||
|
|
||
| // Verify the match candidates info was set | ||
| assertEquals(matchCandidatesInfo, exception.getMatchCandidatesInfo()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testWithNullValue() { | ||
|
Check warning on line 48 in fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/UnableToPlanExceptionTest.java
|
||
| final UnableToPlanException exception = new UnableToPlanException("Test message"); | ||
|
|
||
| // Set to non-null value first | ||
| exception.withMatchCandidatesInfo("match"); | ||
| assertEquals("match", exception.getMatchCandidatesInfo()); | ||
|
|
||
| // Now set to null | ||
| exception.withMatchCandidatesInfo(null); | ||
| assertNull(exception.getMatchCandidatesInfo()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| import com.apple.foundationdb.record.query.plan.cascades.CascadesPlanner; | ||
| import com.apple.foundationdb.record.query.plan.cascades.SemanticException; | ||
| import com.apple.foundationdb.record.query.plan.cascades.StableSelectorCostModel; | ||
| import com.apple.foundationdb.record.query.plan.cascades.UnableToPlanException; | ||
| import com.apple.foundationdb.record.query.plan.cascades.typing.TypeRepository; | ||
| import com.apple.foundationdb.record.query.plan.plans.RecordQueryPlan; | ||
| import com.apple.foundationdb.record.query.plan.serialization.DefaultPlanSerializationRegistry; | ||
|
|
@@ -239,6 +240,25 @@ | |
| planContext.getConstantActionFactory(), planContext.getDbUri(), caseSensitive) | ||
| .generateLogicalPlan(ast.getParseTree())); | ||
| return maybePlan.optimize(planner, planContext, currentPlanHashMode); | ||
| } catch (UnableToPlanException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a test for this? we should test cases where:
|
||
| // Log plan cache information when planning fails | ||
| cache.ifPresent(relationalPlanCache -> logPlanCacheOnFailure(relationalPlanCache, logger)); | ||
|
|
||
| // Log connection options information when planning fails | ||
| if (logger.isErrorEnabled()) { | ||
| final var optionsBuilder = new StringBuilder("Connection Options:\n"); | ||
| for (final var entry : options.entries()) { | ||
| final var key = entry.getKey(); | ||
| final var value = entry.getValue(); | ||
| optionsBuilder.append(String.format(" %s = %s%n", | ||
| key, | ||
| value != null ? value.toString() : "null")); | ||
| } | ||
| logger.error(KeyValueLogMessage.of("Connection options when unable to plan", | ||
| "connectionOptions", optionsBuilder.toString())); | ||
| } | ||
|
|
||
| throw e; | ||
| } catch (MetaDataException mde) { | ||
| // we need a better way for translating error codes between record layer and Relational SQL error codes | ||
| throw new RelationalException(mde.getMessage(), ErrorCode.SYNTAX_OR_ACCESS_VIOLATION, mde).toUncheckedWrappedException(); | ||
|
|
@@ -371,6 +391,44 @@ | |
| return options; | ||
| } | ||
|
|
||
| /** | ||
| * Logs the plan cache state when unable to plan a query. | ||
| * This method collects tertiary cache statistics and logs them for debugging purposes. | ||
| * | ||
| * @param cache The optional plan cache | ||
| * @param logger The logger instance | ||
| */ | ||
| static void logPlanCacheOnFailure(@Nonnull final RelationalPlanCache cache, | ||
| @Nonnull final Logger logger) { | ||
| if (logger.isErrorEnabled()) { | ||
| final var cacheStats = cache.getStats(); | ||
| final var allPrimaryKeys = cacheStats.getAllKeys(); | ||
| final var cacheInfoBuilder = new StringBuilder("Plan Cache Tertiary Layer:\n"); | ||
|
|
||
| for (final var primaryKey : allPrimaryKeys) { | ||
| final var secondaryKeys = cacheStats.getAllSecondaryKeys(primaryKey); | ||
| for (final var secondaryKey : secondaryKeys) { | ||
| final var tertiaryMappings = cacheStats.getAllTertiaryMappings(primaryKey, secondaryKey); | ||
| if (!tertiaryMappings.isEmpty()) { | ||
| cacheInfoBuilder.append(String.format(" [%s][%s]: %d entries%n", primaryKey, secondaryKey, tertiaryMappings.size())); | ||
| for (final var entry : tertiaryMappings.entrySet()) { | ||
| cacheInfoBuilder.append(String.format(" - %s%n", entry.getKey().toString())); | ||
|
Check warning on line 415 in fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/PlanGenerator.java
|
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| cacheInfoBuilder.append(String.format("Cache Stats: size=%d, hits=%d, misses=%d", | ||
| cacheStats.numEntries(), cacheStats.numHits(), cacheStats.numMisses())); | ||
|
|
||
| logger.error(KeyValueLogMessage.of("Plan cache state when unable to plan", | ||
| "planCacheTertiaryLayer", cacheInfoBuilder.toString(), | ||
| "planCachePrimarySize", cacheStats.numEntries(), | ||
| "planCachePrimaryHits", cacheStats.numHits(), | ||
| "planCachePrimaryMisses", cacheStats.numMisses())); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether the query should be looked up and, if not found, cached in the plan cache. Currently, we take | ||
| * this decision is taken statically, in the future we should combine with other combine it with environmental | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this test was actually added to meet TeamScale requirement, added a test around plan cache.