Add instrumentation for DataTable Creation #11942
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11942 +/- ##
============================================
+ Coverage 61.44% 61.49% +0.05%
+ Complexity 1141 207 -934
============================================
Files 2385 2385
Lines 129189 129216 +27
Branches 19998 20001 +3
============================================
+ Hits 79381 79465 +84
+ Misses 44061 43951 -110
- Partials 5747 5800 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 38 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
SabrinaZhaozyf
force-pushed
the
instrument_data_table
branch
from
65a961c
to
bdd423d
Compare
| @@ -215,6 +219,14 @@ private byte[] serializeResponse(ServerQueryRequest queryRequest, InstanceRespon | |||
| byte[] responseByte = null; | |||
| try { | |||
| responseByte = instanceResponse.toDataTable().toBytes(); | |||
There was a problem hiding this comment.
Could we verify whether there's memory allocation in toBytes()? Do we need to instrument that part as well? Meanwhile, IMO it might be helpful to very briefly explain why we only added instrumentation only for groupby and selection operation utils in the PR description @SabrinaZhaozyf
There was a problem hiding this comment.
Could we verify whether there's memory allocation in toBytes()? Do we need to instrument that part as well?
Good point. There's memory allocation in toBytes (mostly from writeLeadingSections) and we have seen OOM here as well. Added instrumentation to serializeStringDictionary in toBytes.
IMO it might be helpful to very briefly explain why we only added instrumentation only for groupby and selection operation utils in the PR description
Done.
| List<DataTable> dataTables = new ArrayList<>(); | ||
| while (numIterations < 100) { | ||
| // build data table with sampling | ||
| dataTables.add(SelectionOperatorUtils.getDataTableFromRows(rows, dataSchema, false)); |
There was a problem hiding this comment.
Could we add DataTable.toBytes() as well here? Meanwhile, the testing would probably fit better to the real use case if we call SelectionOperatorUtils.getDataTableFromRows(rows, dataSchema, false) only once in each thread and submit multiple runner threads. Similar for the groupby case.
There was a problem hiding this comment.
Updated both tests.
SabrinaZhaozyf
force-pushed
the
instrument_data_table
branch
from
c414350
to
d3dc89f
Compare
The failed tests were due to a flaky test which has been fixed here. Thank you for the review! @jasperjiaguo |
When the
InstanceResponseBlockis creating DataTable based on the final results in the combine operator, that data table creation is hitting OOM (see examples below). This PR adds instrumentation for this path under the query killing framework.Instrument is added the impl of
BaseResultsBlock.getDataTable(), specially toGroupByResultsBlock,SelectionResultsBlock, andDistinctResultsBlockwhich are the most memory-intensive.Skipped
AggregationResultsBlock,ExceptionResultsBlock,ExplainResultsBlock,MetadataResultsBlockas they don't have as much memory overhead.Examples from heap dumps
Selection
GroupBy
Testing
ResourceManagerAccountingTestverify that memory usage is sampled, interruption is checked, andEarlyTerminationExceptionis thrown ingetDataTableby manually setting up the accountant after rows/result blocks are created (only samples for data table creation). Without instrumentation, tests will throw OOM exception.Below is an example of query exception in the response when a kill happened exactly at data table creation. While it is not deterministic, to reproduce this, you can set server
accounting.oom.critical.heap.usage.ratioto be higher inOfflineClusterMemBasedServerQueryKillingTestand try runningtestSelectionOnlyOOM.cc @jasperjiaguo @siddharthteotia