Introduce defaultOnDiskStorage config for Group By by capistrant · Pull Request #12833 · apache/druid

capistrant · 2022-07-28T16:17:32Z

Description

This config allows us the cluster operator to set an allowable maxOnDiskStorage value while also providing a default to prevent all queries from using that maximum by default if they don't override to a lower value on the client side via context. This makes sense because an operator may want to allow clients to opt in to a value between 0 and maxOnDiskStorage for their query, while not wanting all queries to use on disk storage by default.

Key changed/added classes in this PR

GroupByQueryConfig

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

FrankChen021 · 2022-07-29T02:14:27Z

  private long maxOnDiskStorage = 0L;

+  @JsonProperty
+  private long defaultOnDiskStorage = Integer.MAX_VALUE;


Both maxOnDiskStorage and this new config can be changed to type of HumanReadableBytes

FrankChen021 · 2022-07-29T02:23:30Z

 |`druid.query.groupBy.maxSelectorDictionarySize`|Maximum amount of heap space (approximately) to use for per-segment string dictionaries. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
 |`druid.query.groupBy.maxMergingDictionarySize`|Maximum amount of heap space (approximately) to use for per-query string dictionaries. When the dictionary exceeds this size, a spill to disk will be triggered. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
 |`druid.query.groupBy.maxOnDiskStorage`|Maximum amount of disk space to use, per-query, for spilling result sets to disk when either the merging buffer or the dictionary fills up. Queries that exceed this limit will fail. Set to zero to disable disk spilling.|0 (disabled)|
+|`druid.query.groupBy.defaultOnDiskStorage`|Default amount of disk space to use, per-query, for spilling the result sets to disk when either the merging buffer or the dictionary fills up. Set to zero to disable disk spilling for queries who don't override `maxOnDiskStorage` in their context.|`druid.query.groupBy.maxOnDiskStorage`|


Suggested change

|`druid.query.groupBy.defaultOnDiskStorage`|Default amount of disk space to use, per-query, for spilling the result sets to disk when either the merging buffer or the dictionary fills up. Set to zero to disable disk spilling for queries who don't override `maxOnDiskStorage` in their context.|`druid.query.groupBy.maxOnDiskStorage`|

|`druid.query.groupBy.defaultOnDiskStorage`|Default amount of disk space to use, per-query, for spilling the result sets to disk when either the merging buffer or the dictionary fills up. Set to zero to disable disk spilling for queries which don't override `maxOnDiskStorage` in their context.|`druid.query.groupBy.maxOnDiskStorage`|

FrankChen021 · 2022-07-29T02:33:00Z

+    // choose a default value lower than the max allowed when the context key is missing in the client query.
    newConfig.maxOnDiskStorage = Math.min(
-        ((Number) query.getContextValue(CTX_KEY_MAX_ON_DISK_STORAGE, getMaxOnDiskStorage())).longValue(),
+        ((Number) query.getContextValue(CTX_KEY_MAX_ON_DISK_STORAGE, getDefaultOnDiskStorage())).longValue(),


If both max and default config are changed to type of HumanReadableBytes, I think we need a new interface(for example getHumanReadableContextValue) on QueryContext to support values in these format:

number

number in string format (in case of Bad error message on incorrect context value #12760)

human readable format

HumanReadableBytes.parse can handle the latter two cases.

FrankChen021 · 2022-07-29T02:33:26Z

  private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;

+
+


accidental changes?

FrankChen021 · 2022-07-29T02:34:34Z

It makes sense to me. Some minor comments are left.

suneet-s · 2022-07-28T20:49:30Z

  private long maxOnDiskStorage = 0L;

+  @JsonProperty
+  private long defaultOnDiskStorage = Integer.MAX_VALUE;


Suggested change

private long defaultOnDiskStorage = Integer.MAX_VALUE;

private long defaultOnDiskStorage = -1;

Since technically Integer.MAX_VALUE seems like a legit config option since defaultOnDiskStorage is a long

suneet-s · 2022-07-28T20:50:05Z

+   */
+  public long getDefaultOnDiskStorage()
+  {
+    return defaultOnDiskStorage == Integer.MAX_VALUE ? getMaxOnDiskStorage() : defaultOnDiskStorage;


Suggested change

return defaultOnDiskStorage == Integer.MAX_VALUE ? getMaxOnDiskStorage() : defaultOnDiskStorage;

return defaultOnDiskStorage < 0 ? getMaxOnDiskStorage() : defaultOnDiskStorage;

suneet-s · 2022-07-29T04:07:32Z

+
+    logger.debug(newConfig.toString());


nit: remove or add something in the log message that explains what the debug message is

Suggested change

logger.debug(newConfig.toString());

suneet-s · 2022-07-29T04:10:18Z

Nice! I was just looking into something like this for a similar use case! :)

FrankChen021

All changes LGTM. Thank you @capistrant

suneet-s · 2022-08-05T21:09:49Z


  boolean getContextBoolean(String key, boolean defaultValue);

+  HumanReadableBytes getContextHumanReadableBytes(String key, HumanReadableBytes defaultValue);


Since Query is marked as an ExtensionPoint I think we should introduce a default implementation of this method to help users who may have extended this class upgrade to this version of Druid without having to make a modification to their extension. The default implementation could be something like

default HumanReadableBytes getContextHumanReadableBytes(String key, HumanReadableBytes defaultValue) { if (getQueryContext != null) { return getQueryContext().getAsHumanReadableBytes(key, defaultValue); } }

Added this default basically word for word. Added a return of the defaultValue if the context doesn't exist

suneet-s

Overall looks great! Just the one comment about the new method in the Query interface

abhishekagarwal87 · 2022-08-08T13:06:30Z

@capistrant - thank you for this feature. is this PR good to be merged? Its green :)

capistrant · 2022-08-08T14:37:26Z

@capistrant - thank you for this feature. is this PR good to be merged? Its green :)

@abhishekagarwal87 I need to address one more thing that @suneet-s called out related to breaking changes of an extension point. Will hopefully have it done today

…iewer

suneet-s

LGTM

Thanks @capistrant !

Lucas.Capistrant added 2 commits July 28, 2022 11:12

Introduce defaultOnDiskStorage config for groupBy

f3b47e6

add debug log to groupby query config

5eb988c

capistrant added Area - Querying Release Notes labels Jul 28, 2022

FrankChen021 reviewed Jul 29, 2022

View reviewed changes

suneet-s reviewed Jul 29, 2022

View reviewed changes

Lucas.Capistrant added 6 commits August 1, 2022 13:09

Apply config change suggestion from review

0330128

Remove accidental new lines

9afffcf

update default value of new default disk storage config

4ea3e99

update debug log to have more descriptive text

a7e4147

Make maxOnDiskStorage and defaultOnDiskStorage HumanRedadableBytes

cb240fe

improve test coverage

7345c7c

FrankChen021 approved these changes Aug 5, 2022

View reviewed changes

suneet-s reviewed Aug 5, 2022

View reviewed changes

Provide default implementation to new default method on advice of rev…

986fab8

…iewer

suneet-s approved these changes Aug 11, 2022

View reviewed changes

suneet-s merged commit 3a3271e into apache:master Aug 12, 2022

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

This was referenced Sep 2, 2022

Bad error message on incorrect context value #12760

Closed

Avoid ClassCastException when getting values from QueryContext #13022

Merged

	\|`druid.query.groupBy.defaultOnDiskStorage`\|Default amount of disk space to use, per-query, for spilling the result sets to disk when either the merging buffer or the dictionary fills up. Set to zero to disable disk spilling for queries who don't override `maxOnDiskStorage` in their context.\|`druid.query.groupBy.maxOnDiskStorage`\|
	\|`druid.query.groupBy.defaultOnDiskStorage`\|Default amount of disk space to use, per-query, for spilling the result sets to disk when either the merging buffer or the dictionary fills up. Set to zero to disable disk spilling for queries which don't override `maxOnDiskStorage` in their context.\|`druid.query.groupBy.maxOnDiskStorage`\|

		private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;

	private long defaultOnDiskStorage = Integer.MAX_VALUE;
	private long defaultOnDiskStorage = -1;

	return defaultOnDiskStorage == Integer.MAX_VALUE ? getMaxOnDiskStorage() : defaultOnDiskStorage;
	return defaultOnDiskStorage < 0 ? getMaxOnDiskStorage() : defaultOnDiskStorage;


		boolean getContextBoolean(String key, boolean defaultValue);

		HumanReadableBytes getContextHumanReadableBytes(String key, HumanReadableBytes defaultValue);

Conversation

capistrant commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankChen021 commented Jul 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suneet-s commented Jul 29, 2022

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 commented Aug 8, 2022

Uh oh!

capistrant commented Aug 8, 2022

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

capistrant commented Jul 28, 2022 •

edited

Loading