[native-pos] Disable AsyncDataCache (thus prefetching)

[Yuhta]

We see negative performance impact (5% ~ 30%) when split prefetch is enabled,
hence adding these config options to disable it.

[shrinidhijoshi]

@Yuhta Please update the commit message (and PR description) with brief explanation of why this change is needed.

[shrinidhijoshi]

We tried to do a test offline but the internal tooling was slowing us down. As this is non-breaking and very specific to cpp process, I am approving this. We can come back and fix if something is missing

[mbasmanova]

@Yuhta Use [native-pos] prefix for Presto-on-Spark specific changes.

[mbasmanova]

@Yuhta What is the definition of num-connector-io-threads property? Is it documented already? If not, would you document it?

[mbasmanova]

We see negative performance impact (5% ~ 30%) when split prefetch is enabled,

What is causing that?

[mbasmanova]

@Yuhta Since you have [native-pos] prefix, adding "for Presto on Spark" is redundant. Let's remove from both PR title and commit message.

[mbasmanova]

We see negative performance impact (5% ~ 30%) when split prefetch is enabled,

@Yuhta What's the cause of performance impact?

[github-actions]

Codenotify: Notifying subscribers in CODENOTIFY files for diff 3d3ee49...ca341eb.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/develop/presto-native.rst

[steveburnett]

Thanks for the documentation! I added a few suggestions to make some statements more explicit and easier to understand. If I have made a suggestion that changes the meaning in a way you disagree with, please help me understand the meaning.

[steveburnett]

LGTM! (docs)

[mbasmanova]

Are we missing some other configs re: SSD. I assume one needs to specify rott directory or something, no?

[Yuhta]

You can use async data cache without SSD

[mbasmanova]

Got it, but if I want to use SSD I assume I need more properties than just size, no?

[Yuhta]

Yes you will need to set SSD size in another property to indicate how much space is available

[mbasmanova]

consistency: async-data-cache-enabled vs. async-cache-ssd-gb - one says data-cache, the other just cache. Would be nice to fix.

[Yuhta]

That will be a big risky change, rather not changing it here

[mbasmanova]

Understood. Perhaps, file a GitHub issue about this.

[Yuhta]

#21185

[mbasmanova]

Should these have default values? true, 0 and 30 (according to documentation).

[Yuhta]

We set it to false here to disable it. The default values are for when it is not overwritten here.

[mbasmanova]

Would you explicitly set it or make a comment that default is 'false'?

private boolean asyncDataCacheEnabled = false;

[mbasmanova]

The default values are for when it is not overwritten here.

I'm not sure I understand.... The user reading the documentation would assume that default is what they get without changing anything. It looks like actual default is false, no?

[Yuhta]

Setting it explicitly violates a lint rule. I will put a comment.

The default is you don't overwrite it in code (i.e., instantiate a plain vanilla PrestoServer). Here POS is overwriting it explicitly so the defaults no longer applies. Do you have a place I can write POS defaults?

[Yuhta]

@mbasmanova I added the comment and add the default value for POS in the documentation in addition to the vanilla Presto

[mbasmanova]

Accepting to unblock, but I'm very confused by how these properties are set and what the defaults are. It would be nice to clear this up in a follow up.

[aditi-pandit]

@Yuhta , @mbasmanova : We noticed this negative performance impact internally for Prestissimo as well. Did you'll only see it for POS, but not Prestissimo ? (Just curious as you are only turning off POS configs)

We should turn it off in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/NativeQueryRunnerUtils.java#L33 config as well. wdyt ?

[Yuhta]

@aditi-pandit We have different deployments of Prestissimo and some do turn the option off. You can use the worker config file to overwrite the system config. POS needs special treatment because the configuration is a little bit weird and worker config does not work out of box there.