-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Extend memory exceeded errors with more details #16297
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| package com.facebook.presto.execution; | ||
|
|
||
| import com.facebook.airlift.concurrent.ThreadPoolExecutorMBean; | ||
| import com.facebook.airlift.json.JsonCodec; | ||
| import com.facebook.airlift.log.Logger; | ||
| import com.facebook.airlift.node.NodeInfo; | ||
| import com.facebook.airlift.stats.CounterStat; | ||
|
|
@@ -37,6 +38,7 @@ | |
| import com.facebook.presto.metadata.MetadataUpdates; | ||
| import com.facebook.presto.operator.ExchangeClientSupplier; | ||
| import com.facebook.presto.operator.FragmentResultCacheManager; | ||
| import com.facebook.presto.operator.TaskMemoryReservationSummary; | ||
| import com.facebook.presto.spi.PrestoException; | ||
| import com.facebook.presto.spi.QueryId; | ||
| import com.facebook.presto.spi.connector.ConnectorMetadataUpdater; | ||
|
|
@@ -77,6 +79,7 @@ | |
| import static com.facebook.presto.SystemSessionProperties.getQueryMaxBroadcastMemory; | ||
| import static com.facebook.presto.SystemSessionProperties.getQueryMaxMemoryPerNode; | ||
| import static com.facebook.presto.SystemSessionProperties.getQueryMaxTotalMemoryPerNode; | ||
| import static com.facebook.presto.SystemSessionProperties.isVerboseExceededMemoryLimitErrorsEnabled; | ||
| import static com.facebook.presto.SystemSessionProperties.resourceOvercommit; | ||
| import static com.facebook.presto.execution.SqlTask.createSqlTask; | ||
| import static com.facebook.presto.memory.LocalMemoryManager.GENERAL_POOL; | ||
|
|
@@ -107,6 +110,7 @@ public class SqlTaskManager | |
| private final Duration clientTimeout; | ||
|
|
||
| private final LocalMemoryManager localMemoryManager; | ||
| private final JsonCodec<List<TaskMemoryReservationSummary>> memoryReservationSummaryJsonCodec; | ||
| private final LoadingCache<QueryId, QueryContext> queryContexts; | ||
| private final LoadingCache<TaskId, SqlTask> tasks; | ||
|
|
||
|
|
@@ -126,6 +130,7 @@ public SqlTaskManager( | |
| SplitMonitor splitMonitor, | ||
| NodeInfo nodeInfo, | ||
| LocalMemoryManager localMemoryManager, | ||
| JsonCodec<List<TaskMemoryReservationSummary>> memoryReservationSummaryJsonCodec, | ||
| TaskManagementExecutor taskManagementExecutor, | ||
| TaskManagerConfig config, | ||
| NodeMemoryConfig nodeMemoryConfig, | ||
|
|
@@ -162,6 +167,7 @@ public SqlTaskManager( | |
| config); | ||
|
|
||
| this.localMemoryManager = requireNonNull(localMemoryManager, "localMemoryManager is null"); | ||
| this.memoryReservationSummaryJsonCodec = requireNonNull(memoryReservationSummaryJsonCodec, "memoryReservationSummaryJsonCodec is null"); | ||
| DataSize maxQueryUserMemoryPerNode = nodeMemoryConfig.getMaxQueryMemoryPerNode(); | ||
| DataSize maxQueryTotalMemoryPerNode = nodeMemoryConfig.getMaxQueryTotalMemoryPerNode(); | ||
| DataSize maxQuerySpillPerNode = nodeSpillConfig.getQueryMaxSpillPerNode(); | ||
|
|
@@ -213,7 +219,8 @@ private QueryContext createQueryContext( | |
| taskNotificationExecutor, | ||
| driverYieldExecutor, | ||
| maxQuerySpillPerNode, | ||
| localSpillManager.getSpillSpaceTracker()); | ||
| localSpillManager.getSpillSpaceTracker(), | ||
| memoryReservationSummaryJsonCodec); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -409,6 +416,8 @@ public TaskInfo updateTask( | |
| } | ||
| } | ||
|
|
||
| queryContext.setVerboseExceededMemoryLimitErrorsEnabled(isVerboseExceededMemoryLimitErrorsEnabled(session)); | ||
|
|
||
|
Comment on lines
419
to
420
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. Wondering why is this done in this way rather than passing it while creating QueryContext object?
Member
Author
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. Unfortunately there's a design flaw in how we create We are already using a similar hack for setting memory limits: https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/execution/SqlTaskManager.java#L405 Perhaps we need to refactor it at some point. But that is probably beyond the scope of this PR. |
||
| sqlTask.recordHeartbeat(); | ||
| return sqlTask.updateTask(session, fragment, sources, outputBuffers, tableWriteInfo); | ||
| } | ||
|
|
||
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.
Maybe rename to EXCEEDED_MEMORY_LIMIT_ERRORS_VERBOSE_LOGGING_ENABLED
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.
EXCEEDED_MEMORY_LIMIT_ERRORS_VERBOSE_LOGGING_ENABLEDmay sound a little misleading, as it suggests that the logging verbosity is getting changed. Setting this property totruewon't increase logging verbosity, but is going to provide extra details in error messages.