Add default configuration for select query 'fromNext' parameter#3986
Add default configuration for select query 'fromNext' parameter#3986fjy merged 4 commits intoapache:masterfrom
Conversation
| } | ||
| ``` | ||
|
|
||
| Note that specifying the `fromNext` option in the `pagingSpec` overrides the default value set by `druid.query.select.enableFromNextDefault` in the server configuration. See [Server configuration](#server-configuration) for more details. |
There was a problem hiding this comment.
These docs, taken as a whole, are kind of confusing. The paragraph above (starting with "Note that in the second query") discusses fromNext in a tone that suggests it's off by default, but then the next section says it's on by default.
Could you reword these docs and the example json to make more sense given that fromNext defaults to true?
I think the only reason anyone would want fromNext to default to false is for compatibility with query tools that don't understand fromNext, and the docs should reflect that.
There was a problem hiding this comment.
I rewrote part of the docs to better explain the query pagination and fromNext, with mention of backwards compatibility
| private final Map<String, Integer> pagingIdentifiers; | ||
| private final int threshold; | ||
| private final boolean fromNext; | ||
| private final Boolean fromNext; |
There was a problem hiding this comment.
Make this boolean and resolve the default fromNext in the constructor, that way it'll be consistent for the entire query when it's forwarded from the broker to the data nodes. (If you don't do this, then when the cluster's data nodes are partially updated, it'll be in a weird state where fromNext is false on some data nodes and true on others for the same query, making it impossible to get correct behavior)
There was a problem hiding this comment.
You might have to @JacksonInject the config to get this to work.
There was a problem hiding this comment.
cool, changed this to resolve fromNext in the constructor
| if (offset == null) { | ||
| offset = PagingOffset.toOffset(0, descending); | ||
| } else if (fromNext) { | ||
| } else if (shouldApplyFromNext(defaultFromNext)) { |
There was a problem hiding this comment.
This can change back to the old logic when fromNext becomes a boolean again.
|
|
||
| public class SelectQueryConfig | ||
| { | ||
| public static String CTX_KEY_ENABLE_FROM_NEXT_DEFAULT = "enableFromNextDefault"; |
There was a problem hiding this comment.
This isn't a context key so it's misleading for it to start with CTX_KEY_. It seems like it's only used in the tests, so IMO it doesn't really need to be a constant at all. But if you want it to be, then please give it another name.
There was a problem hiding this comment.
renamed, removed the "CTX_KEY_" portion
| } | ||
|
|
||
| public byte[] getCacheKey() | ||
| public byte[] getCacheKey(boolean defaultFromNext) |
There was a problem hiding this comment.
This can change back to the old logic when fromNext becomes a boolean again.
|
@gianm I reverted the boolean-related changes in PagingSpec; the changes where I create a SelectQueryConfig supplier in the various unit test suites and SelectQueryToolChest were related to the older patch where I resolved fromNext later and not in the constructor, but I decided to leave these in, I think they could be useful if we add more select config options later on, and I feel like they do no harm there |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Looks good other than the annotations.
| private final Supplier<SelectQueryConfig> configSupplier; | ||
|
|
||
| @JsonCreator | ||
| @Inject |
There was a problem hiding this comment.
You want this injected by Jackson, not a Guice injector, so remove the @Inject annotation and add @JacksonInject to the configSupplier. Check out JavaScriptExtractionFn for an example.
There was a problem hiding this comment.
got it, thanks, fixed this
| Arrays.<String>asList("index"), | ||
| null, | ||
| new PagingSpec(null, 3), | ||
| new PagingSpec(null, 3, null), |
There was a problem hiding this comment.
Might as well make this new PagingSpec(null, 3, true).
There was a problem hiding this comment.
I made that null intentionally for testing that it would grab the default value from the config
|
👍 This was the PR I thought I was merging before accidentally merging #3989 |
3 participants
This PR adds a configuration option that sets a default value for the
fromNextproperty of the SelectQuery'spagingSpec.If
fromNextis omitted, the value set bydruid.query.select.enableFromNextDefaultwill be used.This is a continuation of #2767 by @navis, based on the discussion in that thread (see #2767 (comment))