drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jacq...@apache.org
Subject drill git commit: DRILL-2750: Running 1 or more queries against Drillbits having insufficient DirectMem renders the Drillbits in an unusable state
Date Wed, 13 May 2015 14:16:29 GMT
Repository: drill
Updated Branches:
  refs/heads/master 880dff8e2 -> d1526f946


DRILL-2750: Running 1 or more queries against Drillbits having insufficient DirectMem renders
the Drillbits in an unusable state

Fix memory leaks found at operator tree construction time if there is an
allocation failure.


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/d1526f94
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/d1526f94
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/d1526f94

Branch: refs/heads/master
Commit: d1526f9462f6817a76631464ff332bb99b3bdf28
Parents: 880dff8
Author: Chris Westin <cwestin@yahoo.com>
Authored: Tue May 12 21:00:57 2015 -0700
Committer: Jacques Nadeau <jacques@apache.org>
Committed: Wed May 13 05:47:20 2015 -0700

----------------------------------------------------------------------
 .../org/apache/drill/common/AutoCloseables.java |  2 +-
 .../drill/exec/memory/TopLevelAllocator.java    |  2 +-
 .../drill/exec/physical/impl/ImplCreator.java   | 35 ++++++++----
 .../impl/aggregate/HashAggTemplate.java         | 59 +++++++++++---------
 .../physical/impl/common/HashTableTemplate.java | 47 ++++++++++------
 .../physical/impl/xsort/ExternalSortBatch.java  | 39 +++++++------
 6 files changed, 108 insertions(+), 76 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/common/src/main/java/org/apache/drill/common/AutoCloseables.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/AutoCloseables.java b/common/src/main/java/org/apache/drill/common/AutoCloseables.java
index 39c5d78..fa1eb92 100644
--- a/common/src/main/java/org/apache/drill/common/AutoCloseables.java
+++ b/common/src/main/java/org/apache/drill/common/AutoCloseables.java
@@ -39,7 +39,7 @@ public class AutoCloseables {
     try {
       ac.close();
     } catch(Exception e) {
-      logger.info("Failure on close(): " + e);
+      logger.warn("Failure on close(): " + e);
     }
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
index a78deb6..9670c7e 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java
@@ -289,7 +289,7 @@ public class TopLevelAllocator implements BufferAllocator {
           if (!child.isClosed()) {
             StringBuilder sb = new StringBuilder();
             StackTraceElement[] elements = children.get(child);
-            for (int i = 3; i < elements.length; i++) {
+            for (int i = 1; i < elements.length; i++) {
               sb.append("\t\t");
               sb.append(elements[i]);
               sb.append("\n");

http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
index 5cea748..77ca0f5 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ImplCreator.java
@@ -17,12 +17,14 @@
  */
 package org.apache.drill.exec.physical.impl;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.security.PrivilegedExceptionAction;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 
+import org.apache.drill.common.AutoCloseables;
 import org.apache.drill.common.exceptions.ExecutionSetupException;
 import org.apache.drill.exec.ops.FragmentContext;
 import org.apache.drill.exec.physical.base.FragmentRoot;
@@ -74,21 +76,30 @@ public class ImplCreator {
     final ImplCreator creator = new ImplCreator();
     Stopwatch watch = new Stopwatch();
     watch.start();
-    final RootExec rootExec = creator.getRootExec(root, context);
 
-    // skip over this for SimpleRootExec (testing)
-    if (rootExec instanceof BaseRootExec) {
-      ((BaseRootExec) rootExec).setOperators(creator.getOperators());
-    }
-
-    logger.debug("Took {} ms to create RecordBatch tree", watch.elapsed(TimeUnit.MILLISECONDS));
-    if (rootExec == null) {
-      throw new ExecutionSetupException(
-          "The provided fragment did not have a root node that correctly created a RootExec
value.");
-    }
+    boolean success = false;
+    try {
+      final RootExec rootExec = creator.getRootExec(root, context);
+      // skip over this for SimpleRootExec (testing)
+      if (rootExec instanceof BaseRootExec) {
+        ((BaseRootExec) rootExec).setOperators(creator.getOperators());
+      }
 
-    return rootExec;
+      logger.debug("Took {} ms to create RecordBatch tree", watch.elapsed(TimeUnit.MILLISECONDS));
+      if (rootExec == null) {
+        throw new ExecutionSetupException(
+            "The provided fragment did not have a root node that correctly created a RootExec
value.");
+      }
 
+      success = true;
+      return rootExec;
+    } finally {
+      if (!success) {
+        for(final CloseableRecordBatch crb : creator.getOperators()) {
+          AutoCloseables.close(crb, logger);
+        }
+      }
+    }
   }
 
   /** Create RootExec and its children (RecordBatches) for given FragmentRoot */

http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
index e92de40..8af1508 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
@@ -128,34 +128,41 @@ public abstract class HashAggTemplate implements HashAggregator {
     private BatchHolder() {
 
       aggrValuesContainer = new VectorContainer();
+      boolean success = false;
+      try {
+        ValueVector vector;
+
+        for (int i = 0; i < materializedValueFields.length; i++) {
+          MaterializedField outputField = materializedValueFields[i];
+          // Create a type-specific ValueVector for this value
+          vector = TypeHelper.getNewVector(outputField, allocator);
+
+          // Try to allocate space to store BATCH_SIZE records. Key stored at index i in
HashTable has its workspace
+          // variables (such as count, sum etc) stored at index i in HashAgg. HashTable and
HashAgg both have
+          // BatchHolders. Whenever a BatchHolder in HashAgg reaches its capacity, a new
BatchHolder is added to
+          // HashTable. If HashAgg can't store BATCH_SIZE records in a BatchHolder, it leaves
empty slots in current
+          // BatchHolder in HashTable, causing the HashTable to be space inefficient. So
it is better to allocate space
+          // to fit as close to as BATCH_SIZE records.
+          if (vector instanceof FixedWidthVector) {
+            ((FixedWidthVector) vector).allocateNew(HashTable.BATCH_SIZE);
+          } else if (vector instanceof VariableWidthVector) {
+            ((VariableWidthVector) vector).allocateNew(HashTable.VARIABLE_WIDTH_VECTOR_SIZE
* HashTable.BATCH_SIZE,
+                HashTable.BATCH_SIZE);
+          } else if (vector instanceof ObjectVector) {
+            ((ObjectVector) vector).allocateNew(HashTable.BATCH_SIZE);
+          } else {
+            vector.allocateNew();
+          }
 
-      ValueVector vector;
-
-      for (int i = 0; i < materializedValueFields.length; i++) {
-        MaterializedField outputField = materializedValueFields[i];
-        // Create a type-specific ValueVector for this value
-        vector = TypeHelper.getNewVector(outputField, allocator);
-
-        // Try to allocate space to store BATCH_SIZE records. Key stored at index i in HashTable
has its workspace
-        // variables (such as count, sum etc) stored at index i in HashAgg. HashTable and
HashAgg both have
-        // BatchHolders. Whenever a BatchHolder in HashAgg reaches its capacity, a new BatchHolder
is added to
-        // HashTable. If HashAgg can't store BATCH_SIZE records in a BatchHolder, it leaves
empty slots in current
-        // BatchHolder in HashTable, causing the HashTable to be space inefficient. So it
is better to allocate space
-        // to fit as close to as BATCH_SIZE records.
-        if (vector instanceof FixedWidthVector) {
-          ((FixedWidthVector) vector).allocateNew(HashTable.BATCH_SIZE);
-        } else if (vector instanceof VariableWidthVector) {
-          ((VariableWidthVector) vector).allocateNew(HashTable.VARIABLE_WIDTH_VECTOR_SIZE
* HashTable.BATCH_SIZE,
-              HashTable.BATCH_SIZE);
-        } else if (vector instanceof ObjectVector) {
-          ((ObjectVector) vector).allocateNew(HashTable.BATCH_SIZE);
-        } else {
-          vector.allocateNew();
-        }
-
-        capacity = Math.min(capacity, vector.getValueCapacity());
+          capacity = Math.min(capacity, vector.getValueCapacity());
 
-        aggrValuesContainer.add(vector);
+          aggrValuesContainer.add(vector);
+        }
+        success = true;
+      } finally {
+        if (!success) {
+          aggrValuesContainer.clear();
+        }
       }
     }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
index 0908e50..e0876af 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
@@ -123,27 +123,38 @@ public abstract class HashTableTemplate implements HashTable {
       this.batchIndex = idx;
 
       htContainer = new VectorContainer();
-      for (VectorWrapper<?> w : htContainerOrig) {
-        ValueVector vv = TypeHelper.getNewVector(w.getField(), allocator);
-
-        // Capacity for "hashValues" and "links" vectors is BATCH_SIZE records. It is better
to allocate space for
-        // "key" vectors to store as close to as BATCH_SIZE records. A new BatchHolder is
created when either BATCH_SIZE
-        // records are inserted or "key" vectors ran out of space. Allocating too less space
for "key" vectors will
-        // result in unused space in "hashValues" and "links" vectors in the BatchHolder.
Also for each new
-        // BatchHolder we create a SV4 vector of BATCH_SIZE in HashJoinHelper.
-        if (vv instanceof FixedWidthVector) {
-          ((FixedWidthVector) vv).allocateNew(BATCH_SIZE);
-        } else if (vv instanceof VariableWidthVector) {
-          ((VariableWidthVector) vv).allocateNew(VARIABLE_WIDTH_VECTOR_SIZE, BATCH_SIZE);
-        } else {
-          vv.allocateNew();
+      boolean success = false;
+      try {
+        for (VectorWrapper<?> w : htContainerOrig) {
+          ValueVector vv = TypeHelper.getNewVector(w.getField(), allocator);
+
+          // Capacity for "hashValues" and "links" vectors is BATCH_SIZE records. It is better
to allocate space for
+          // "key" vectors to store as close to as BATCH_SIZE records. A new BatchHolder
is created when either BATCH_SIZE
+          // records are inserted or "key" vectors ran out of space. Allocating too less
space for "key" vectors will
+          // result in unused space in "hashValues" and "links" vectors in the BatchHolder.
Also for each new
+          // BatchHolder we create a SV4 vector of BATCH_SIZE in HashJoinHelper.
+          if (vv instanceof FixedWidthVector) {
+            ((FixedWidthVector) vv).allocateNew(BATCH_SIZE);
+          } else if (vv instanceof VariableWidthVector) {
+            ((VariableWidthVector) vv).allocateNew(VARIABLE_WIDTH_VECTOR_SIZE, BATCH_SIZE);
+          } else {
+            vv.allocateNew();
+          }
+
+          htContainer.add(vv);
         }
 
-        htContainer.add(vv);
+        links = allocMetadataVector(HashTable.BATCH_SIZE, EMPTY_SLOT);
+        hashValues = allocMetadataVector(HashTable.BATCH_SIZE, 0);
+        success = true;
+      } finally {
+        if (!success) {
+          htContainer.clear();
+          if (links != null) {
+            links.clear();
+          }
+        }
       }
-
-      links = allocMetadataVector(HashTable.BATCH_SIZE, EMPTY_SLOT);
-      hashValues = allocMetadataVector(HashTable.BATCH_SIZE, 0);
     }
 
     private void init(IntVector links, IntVector hashValues, int size) {

http://git-wip-us.apache.org/repos/asf/drill/blob/d1526f94/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
index 612777e..529a6ca 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
@@ -149,27 +149,30 @@ public class ExternalSortBatch extends AbstractRecordBatch<ExternalSort>
{
 
   @Override
   public void close() {
-    if (batchGroups != null) {
-      for (BatchGroup group: batchGroups) {
-        try {
-          group.cleanup();
-        } catch (IOException e) {
-          throw new RuntimeException(e);
+    try {
+      if (batchGroups != null) {
+        for (BatchGroup group: batchGroups) {
+          try {
+            group.cleanup();
+          } catch (IOException e) {
+            throw new RuntimeException(e);
+          }
         }
       }
+    } finally {
+      if (builder != null) {
+        builder.clear();
+        builder.close();
+      }
+      if (sv4 != null) {
+        sv4.clear();
+      }
+      if (copier != null) {
+        copier.cleanup();
+      }
+      copierAllocator.close();
+      super.close();
     }
-    if (builder != null) {
-      builder.clear();
-      builder.close();
-    }
-    if (sv4 != null) {
-      sv4.clear();
-    }
-    if (copier != null) {
-      copier.cleanup();
-    }
-    copierAllocator.close();
-    super.close();
   }
 
   @Override


Mime
View raw message