Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ben-Zvi
Copy link
Contributor

@Ben-Zvi Ben-Zvi commented Jun 29, 2018

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.

@Ben-Zvi Ben-Zvi requested a review from ilooner June 29, 2018 03:15
@Ben-Zvi Ben-Zvi force-pushed the DRILL-6543 branch 2 times, most recently from 44e0786 to bba66a7 Compare July 3, 2018 01:28
* <p>
* DEFAULT: 25%
* </p>
*/
Copy link
Contributor

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);
Copy link
Contributor

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 ) {
Copy link
Contributor

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));

Copy link
Contributor Author

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);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants