-
Notifications
You must be signed in to change notification settings - Fork 979
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
DRILL-6543: Disable Hash Join fallback, add percent_reserved_allowance_from_direct #1351
base: master
Are you sure you want to change the base?
Conversation
44e0786
to
bba66a7
Compare
* <p> | ||
* DEFAULT: 25% | ||
* </p> | ||
*/ |
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.
Thanks for adding this documentation.
*/ | ||
@Test | ||
public void testReservedAllowanceCheck() throws Exception { | ||
OperatorFixture.Builder builder = OperatorFixture.builder(dirTestWatcher); |
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.
Thanks for adding a unit test.
double percent_allowance = optionManager.getOption(ExecConstants.PERCENT_RESERVED_ALLOWANCE_FROM_DIRECT); | ||
|
||
// verify that the allowance is kept | ||
if ( percent_per_query + percent_allowance > 1.0 ) { |
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.
Why do we need to make sure these add to one? Couldn't we just reserve our allowance from the previously computed maxAllocPerNode value. For example
double percentAllowance = optionManager.getOption(ExecConstants.PERCENT_RESERVED_ALLOWANCE_FROM_DIRECT);
// Memory computed as a percent of total memory.
long perQueryMemory = Math.round(directMemory *
optionManager.getOption(ExecConstants.PERCENT_MEMORY_PER_QUERY));
// But, must allow at least the amount given explicitly for
// backward compatibility.
perQueryMemory = Math.max(perQueryMemory,
optionManager.getOption(ExecConstants.MAX_QUERY_MEMORY_PER_NODE));
// Compute again as either the total direct memory, or the
// configured maximum top-level allocation (10 GB).
long maxAllocPerNode = Math.min(directMemory,
config.getLong(RootAllocatorFactory.TOP_LEVEL_MAX_ALLOC));
// Final amount per node per query is the minimum of these two.
maxAllocPerNode = Math.min(maxAllocPerNode, perQueryMemory);
// Deduct non buffered allowance
return Math.round(maxAllocPerNode * (1.0 - percentAllowance));
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.
The code in the PR "enforces" keeping the allowance (out of the direct memory); any user settings that violates this limit returns an error (unfortunately can only be done when a query (using buffered ops) is executed).
This mainly serves as a reminder (for the common user that does not read the documentation). It does not help with concurrent query execution (though users using concurrent are usually more sophisticated, so may know about the allowance).
The example suggested above applies the allowance onto the final computed memory, which is not the intention. For example, Direct Memory of 20GB, and Mem Per Query of 4GB -- then the code about would subtract 25% and the Mem Per Query would be only 3GB (and confuse the user).
@@ -138,16 +139,36 @@ public static long computeOperatorMemory(OptionSet optionManager, long maxAllocP | |||
@VisibleForTesting | |||
public static long computeQueryMemory(DrillConfig config, OptionSet optionManager, long directMemory) { | |||
|
|||
// Get the options | |||
double percent_per_query = optionManager.getOption(ExecConstants.PERCENT_MEMORY_PER_QUERY); |
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.
Minor nitpick, but the convention in Java is to use Camel case for names
percentPerQuery
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.
Fixed ....
Added a new option: planner.memory.percent_reserved_allowance_from_direct
This new option enforces (when a query with buffered operators is run) reserving part of the Direct Memory for the non-buffered operators. It checks against the two ways to set the memory (for the buffered operators): Memory per query per node, or Percent per query.
Also added a test for the enforcement.