drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From paul-rogers <...@git.apache.org>
Subject [GitHub] drill pull request #938: DRILL-5694: Handle HashAgg OOM by spill and retry, ...
Date Sun, 10 Sep 2017 02:53:47 GMT
Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/938#discussion_r137939253
  
    --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
---
    @@ -382,19 +390,25 @@ private void delayedSetup() {
         final boolean fallbackEnabled = context.getOptions().getOption(ExecConstants.HASHAGG_FALLBACK_ENABLED_KEY).bool_val;
     
         // Set the number of partitions from the configuration (raise to a power of two,
if needed)
    -    numPartitions = context.getConfig().getInt(ExecConstants.HASHAGG_NUM_PARTITIONS);
    -    if ( numPartitions == 1 ) {
    +    numPartitions = (int)context.getOptions().getOption(ExecConstants.HASHAGG_NUM_PARTITIONS_VALIDATOR);
    +    if ( numPartitions == 1 && is2ndPhase  ) { // 1st phase can still do early
return with 1 partition
           canSpill = false;
           logger.warn("Spilling is disabled due to configuration setting of num_partitions
to 1");
         }
         numPartitions = BaseAllocator.nextPowerOfTwo(numPartitions); // in case not a power
of 2
     
    -    if ( schema == null ) { estMaxBatchSize = 0; } // incoming was an empty batch
    +    if ( schema == null ) { estValuesBatchSize = estOutgoingAllocSize = estMaxBatchSize
= 0; } // incoming was an empty batch
         else {
           // Estimate the max batch size; should use actual data (e.g. lengths of varchars)
           updateEstMaxBatchSize(incoming);
         }
    -    long memAvail = memoryLimit - allocator.getAllocatedMemory();
    +    // create "reserved memory" and adjust the memory limit down
    +    reserveValueBatchMemory = reserveOutgoingMemory = estValuesBatchSize ;
    +    long newMemoryLimit = allocator.getLimit() - reserveValueBatchMemory - reserveOutgoingMemory
;
    +    long memAvail = newMemoryLimit - allocator.getAllocatedMemory();
    +    if ( memAvail <= 0 ) { throw new OutOfMemoryException("Too little memory available");
}
    +    allocator.setLimit(newMemoryLimit);
    +
    --- End diff --
    
    This code has grown to be incredibly complex with many, many paths through the various
functions.
    
    Tests are handy things. Do we have system-level unit tests that exercise each path through
the code? Otherwise, as a reviewer, how can I be sure that each execution path does, in fact,
work?


---

Mime
View raw message