From 25a7f9ed45b97f9be5971fa979c1b408a6311d8e Mon Sep 17 00:00:00 2001 From: Paul Rogers Date: Tue, 13 Dec 2016 14:36:42 -0800 Subject: [PATCH 1/2] DRILL-5104: Foreman should not set external sort memory for a physical plan Physical plans include a plan for memory allocations. However, the code path in Foreman replans external sort memory, even for a physical plan. This makes it impossible to use a physical plan to test memory configuration. This change avoids changing memory settings in a physical plan; while preserving the adjustments for logical plans or SQL queries. Revised to put a property in the plan itself. Old plans, and those generated from SQL, will have memory allocations applied. Plans marked as already "resource management" planned will be used as-is. Includes a unit test that demonstrates the new behavior. --- .../drill/exec/physical/PhysicalPlan.java | 3 --- .../exec/util/MemoryAllocationUtilities.java | 9 ++++++++- .../drill/exec/work/foreman/Foreman.java | 1 + .../impl/xsort/TestSimpleExternalSort.java | 6 ++++-- .../xsort/one_key_sort_descending.json | 3 ++- .../drill/common/logical/PlanProperties.java | 18 ++++++++++++++++-- 6 files changed, 31 insertions(+), 9 deletions(-) diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/PhysicalPlan.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/PhysicalPlan.java index 78b882b79f1..e0902c84bfe 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/PhysicalPlan.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/PhysicalPlan.java @@ -62,10 +62,8 @@ public List getSortedOperators(boolean reverse){ }else{ return list; } - } - @JsonProperty("head") public PlanProperties getProperties() { return properties; @@ -89,5 +87,4 @@ public String unparse(ObjectWriter writer) { throw new RuntimeException(e); } } - } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java b/exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java index 38dfcd054d0..678167f1d0d 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/util/MemoryAllocationUtilities.java @@ -41,6 +41,13 @@ public class MemoryAllocationUtilities { * @param queryContext */ public static void setupSortMemoryAllocations(final PhysicalPlan plan, final QueryContext queryContext) { + + // Test plans may already have a pre-defined memory plan. + // Otherwise, determine memory allocation. + + if (plan.getProperties().hasResourcePlan) { + return; + } // look for external sorts final List sortList = new LinkedList<>(); for (final PhysicalOperator op : plan.getSortedOperators()) { @@ -64,6 +71,6 @@ public static void setupSortMemoryAllocations(final PhysicalPlan plan, final Que externalSort.setMaxAllocation(maxSortAlloc); } } + plan.getProperties().hasResourcePlan = true; } - } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java b/exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java index c6a31042836..30718b6ef94 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java @@ -774,6 +774,7 @@ private void logQuerySummary() { } } + @SuppressWarnings("resource") @Override public void close() { Preconditions.checkState(!isClosed); diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java index b34a4667d5b..85975cb7780 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java @@ -42,7 +42,6 @@ import com.google.common.base.Charsets; import com.google.common.io.Files; -@Ignore public class TestSimpleExternalSort extends BaseTestQuery { static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestSimpleExternalSort.class); DrillConfig c = DrillConfig.create(); @@ -50,6 +49,7 @@ public class TestSimpleExternalSort extends BaseTestQuery { @Rule public final TestRule TIMEOUT = TestTools.getTimeoutRule(80000); + @Ignore @Test public void mergeSortWithSv2() throws Exception { List results = testPhysicalFromFileWithResults("xsort/one_key_sort_descending_sv2.json"); @@ -109,7 +109,7 @@ public void sortOneKeyDescendingMergeSort() throws Throwable{ for (QueryDataBatch b : results) { if (b.getHeader().getRowCount() == 0) { - break; + continue; } batchCount++; RecordBatchLoader loader = new RecordBatchLoader(allocator); @@ -132,6 +132,7 @@ public void sortOneKeyDescendingMergeSort() throws Throwable{ } @Test + @Ignore public void sortOneKeyDescendingExternalSort() throws Throwable{ RemoteServiceSet serviceSet = RemoteServiceSet.getLocalServiceSet(); @@ -186,6 +187,7 @@ public void sortOneKeyDescendingExternalSort() throws Throwable{ } @Test + @Ignore public void outOfMemoryExternalSort() throws Throwable{ RemoteServiceSet serviceSet = RemoteServiceSet.getLocalServiceSet(); diff --git a/exec/java-exec/src/test/resources/xsort/one_key_sort_descending.json b/exec/java-exec/src/test/resources/xsort/one_key_sort_descending.json index f4eab5d2f3a..b4794adc70e 100644 --- a/exec/java-exec/src/test/resources/xsort/one_key_sort_descending.json +++ b/exec/java-exec/src/test/resources/xsort/one_key_sort_descending.json @@ -4,7 +4,8 @@ version:"1", generator:{ type:"manual" - } + }, + hasResourcePlan: true }, graph:[ { diff --git a/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java b/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java index ce9603e3515..f4de0eba8fa 100644 --- a/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java +++ b/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java @@ -35,6 +35,12 @@ public static enum PlanType {APACHE_DRILL_LOGICAL, APACHE_DRILL_PHYSICAL} public JSONOptions options; public int queue; + /** + * Indicates if the plan has been planned for resource management + * (memory, etc.) or if this plan must still be computed. + */ + public boolean hasResourcePlan; + // @JsonInclude(Include.NON_NULL) public static class Generator { public String type; @@ -55,7 +61,8 @@ private PlanProperties(@JsonProperty("version") int version, @JsonProperty("type") PlanType type, @JsonProperty("mode") ResultMode resultMode, @JsonProperty("options") JSONOptions options, - @JsonProperty("queue") int queue + @JsonProperty("queue") int queue, + @JsonProperty("hasResourcePlan") boolean hasResourcePlan ) { this.version = version; this.queue = queue; @@ -63,6 +70,7 @@ private PlanProperties(@JsonProperty("version") int version, this.type = type; this.resultMode = resultMode == null ? ResultMode.EXEC : resultMode; this.options = options; + this.hasResourcePlan = hasResourcePlan; } public static PlanPropertiesBuilder builder() { @@ -76,6 +84,7 @@ public static class PlanPropertiesBuilder { private ResultMode mode = ResultMode.EXEC; private JSONOptions options; private int queueNumber = 0; + private boolean hasResourcePlan = false; public PlanPropertiesBuilder type(PlanType type) { this.type = type; @@ -112,8 +121,13 @@ public PlanPropertiesBuilder generator(Generator generator) { return this; } + public PlanPropertiesBuilder generator(boolean hasResourcePlan) { + this.hasResourcePlan = hasResourcePlan; + return this; + } + public PlanProperties build() { - return new PlanProperties(version, generator, type, mode, options, queueNumber); + return new PlanProperties(version, generator, type, mode, options, queueNumber, hasResourcePlan); } } From 3f1d35b404c8479b0933587b3db379e4208a1a96 Mon Sep 17 00:00:00 2001 From: Paul Rogers Date: Wed, 21 Dec 2016 18:48:53 -0800 Subject: [PATCH 2/2] Fix typo in method name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Victim of copy & paste… Fixed method name as suggested by reviewer. --- .../java/org/apache/drill/common/logical/PlanProperties.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java b/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java index f4de0eba8fa..b5071779dda 100644 --- a/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java +++ b/logical/src/main/java/org/apache/drill/common/logical/PlanProperties.java @@ -121,7 +121,7 @@ public PlanPropertiesBuilder generator(Generator generator) { return this; } - public PlanPropertiesBuilder generator(boolean hasResourcePlan) { + public PlanPropertiesBuilder hasResourcePlan(boolean hasResourcePlan) { this.hasResourcePlan = hasResourcePlan; return this; }