Add ability to disable partitions caching in hive metastore by s2lomon · Pull Request #12343 · trinodb/trino

s2lomon · 2022-05-12T00:53:12Z

Add ability to disable partitions caching in hive metastore

Thanks to this, one can disable caching of partitions, that
should fix issues with missing partitions during query
execution.

Description

Is this change a fix, improvement, new feature, refactoring, or other?

It's a new feature that helps fixing some issues with inconsistencies in hive metastore caching

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

It's a change to a connector

How would you describe this change to a non-technical end user or system administrator?

It allows you to pick whether you want to cache ALL, DOWN TO TABLE, or PARTITION metadata for hive metastore.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

kokosing

Missing tests. But please

kokosing · 2022-05-12T09:29:47Z

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java

Do you need TABLE now?

kokosing · 2022-05-12T09:30:26Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

add a method to enum instead

scope.matches(otherScope);

kokosing · 2022-05-12T09:31:51Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

I would enable this only when ALL

kokosing · 2022-05-12T09:31:57Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

I would enable this only when ALL

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

findepi · 2022-05-13T08:07:52Z

It's a new feature that helps fixing some issues with inconsistencies in hive metastore caching

What is the use-case scenario when a user would want/need to use this?

Praveen2112 · 2022-05-16T05:42:53Z

What is the use-case scenario when a user would want/need to use this?

It allows user to partially discard a cache - like we would like to cache the table details but not the partition information (if the user adds/drops a partition frequently)

findepi · 2022-05-16T13:25:37Z

What is the use-case scenario when a user would want/need to use this?

It allows user to partially discard a cache - like we would like to cache the table details but not the partition information (if the user adds/drops a partition frequently)

I know what the change does on a technical level.

It does not answer my question about a use-case scenario when a user would want/need to use this.

Praveen2112 · 2022-05-16T14:46:35Z

It does not answer my question about a use-case scenario when a user would want/need to use this.

In case of new partitions being added or dropped at some irregular interval - would make the system unusable at times at it fails (if partitions are dropped). We need to either disable the cache fully - but we might hit the underlying metastore too many times. This would be kind of a sweet spot between both the worlds.

Praveen2112 · 2022-05-16T14:28:27Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

can we move this to a next line ?

Praveen2112 · 2022-05-16T14:29:22Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

Is this change required as a part of this commit ?

nope, it's my mistake

Praveen2112 · 2022-05-16T14:47:44Z

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

Is there is any expansion for sutAction ?

sut is a shortcut for system under test. It's a way to mark this as a holder for all actions that are actually being tested. I could rename it to cachingHiveMetastoreInteractions or metastoreInteractions/Actions. For me sut is ok, but maybe the fact that you need to know what it means makes it worst than one of the above.

sut is a shortcut for system under test.

would never guess

findepi · 2022-05-17T11:18:45Z

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java

hive.metastore-cache-partitions

however, we should have hive.metastore-cache.* prefix, so maybe hive.metastore-cache.cache-partitions and rename existing configs in a follow-up.

cc @losipiuk @electrum

i suggest hive.metastore-cache.cache-partitions and rename existing configs in a follow-up.

Ok we can do it this way.

findepi · 2022-05-17T11:18:57Z

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java

put getter above the setter

findepi · 2022-05-17T11:19:54Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

use actual default: new ..Config().is...CacheaPartitions()

Ok so when I've wanted to change this it hit me as a very bad practice. I understand that the reasoning for keeping the value automated on the test makes it easier to maintain changes in the default value of this field. Still I feel that making it so defeats the purpose of having these tests anyway (and makes it harder to read). I would argue that in the test we maintain automatically checked contract of the configuration of Trino, so that whenever we change defaults like this, we need to at least make it intentionally and this tests guards us from doing such changes by mistake.

Otherwise we could derive all our defaults from the inlined instance of the class, which would mean that we are writing tests for defaults only because we have a checkstyle for that...

@s2lomon
if this tests's purpose is to test things related to partitions being cached or not, then i agree, the config should be set explicitly.
however, it's not the goal here. you supplied the config only because the compiler ordered you to (missing parameter), not that because you really wanted to enable or disable partition caching.
new ..Config().is...CacheaPartitions() conveys this intent, and neither true nor false conveys that intent

switch to new ..Config().is...CacheaPartitions() please

Omg you are right, for some reason I've thought that this comment was about the default test. Good point and thanks for being patient :)

findepi · 2022-05-17T11:20:26Z

...-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastoreConfig.java

You chose to add the field as the last one in the config class, so add it as a last one in the test class too.
(or move declaration in the config class)

(same below)

findepi · 2022-05-17T11:21:35Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

cmt msg

Thanks to this, one can disable caching of partitions, that
should fix issues with missing partitions during query
execution.

Mention the fact this is for the case where table partitions are modified externally.

Sure thing.

s2lomon · 2022-05-19T09:17:38Z

It does not answer my question about a use-case scenario when a user would want/need to use this.

In case of new partitions being added or dropped at some irregular interval - would make the system unusable at times at it fails (if partitions are dropped). We need to either disable the cache fully - but we might hit the underlying metastore too many times. This would be kind of a sweet spot between both the worlds.

I guess it should fix issues like this one. #6286 Or at least would be a good try for such scenarios. Alternatively we could add fine tuning parameters, but I guess it's easier to have it cached as always or not at all.

findepi · 2022-05-19T10:45:29Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

the executor (refresh executor) is useless for a "never cache", drop the param

So Optional.empty()? ok.

findepi · 2022-05-19T10:45:57Z

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java

i suggest hive.metastore-cache.cache-partitions and rename existing configs in a follow-up.

findepi · 2022-05-19T10:46:15Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

switch to new ..Config().is...CacheaPartitions() please

findepi · 2022-05-19T10:47:41Z

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

do ordinary for

findepi · 2022-05-19T10:47:44Z

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

do ordinary for

findepi · 2022-05-19T10:48:46Z

...-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastoreConfig.java

make it last (as everywhere else)

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java

Thanks to this, one can disable caching of partitions, that should fix issues with missing partitions during query execution. This can happen when partitions are often replaced or changed by some automated external processes.

findepi · 2022-05-24T14:37:44Z

CI #11275 (reopened)

mosabua · 2022-05-24T16:52:23Z

@colebow can you verify if we need docs and if yes .. raise a PR asap.

cla-bot bot added the cla-signed label May 12, 2022

s2lomon requested review from Praveen2112 and kokosing May 12, 2022 00:56

github-actions bot added the tests:hive label May 12, 2022

kokosing reviewed May 12, 2022

View reviewed changes

s2lomon force-pushed the hive-metastore-partitions-cache branch from 2aeaa77 to 3040352 Compare May 16, 2022 12:10

s2lomon changed the title ~~Add MetastoreCacheScope to caching hive metastore config~~ Add ability to disable partitions caching in hive metastore May 16, 2022

Praveen2112 reviewed May 16, 2022

View reviewed changes

findepi approved these changes May 17, 2022

View reviewed changes

s2lomon force-pushed the hive-metastore-partitions-cache branch from 3040352 to 7cf7803 Compare May 19, 2022 09:44

findepi reviewed May 19, 2022

View reviewed changes

Praveen2112 approved these changes May 20, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java Outdated Show resolved Hide resolved

s2lomon force-pushed the hive-metastore-partitions-cache branch from 7cf7803 to 4e4945e Compare May 23, 2022 17:50

findepi approved these changes May 24, 2022

View reviewed changes

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastoreConfig.java Outdated Show resolved Hide resolved

Add ability to disable partitions caching in hive metastore

f89babb

Thanks to this, one can disable caching of partitions, that should fix issues with missing partitions during query execution. This can happen when partitions are often replaced or changed by some automated external processes.

s2lomon force-pushed the hive-metastore-partitions-cache branch from 4e4945e to f89babb Compare May 24, 2022 10:17

findepi approved these changes May 24, 2022

View reviewed changes

findepi merged commit 7834be9 into trinodb:master May 24, 2022

findepi mentioned this pull request May 24, 2022

Release notes for 382 #12425

Closed

github-actions bot added this to the 382 milestone May 24, 2022

mosabua mentioned this pull request May 24, 2022

Add Trino 382 release notes #12440

Merged

colebow mentioned this pull request May 24, 2022

Add docs for hive partition caching property #12534

Merged

findepi mentioned this pull request May 27, 2022

Remove HiveConfig from Iceberg module #12506

Merged

Conversation

s2lomon commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

kokosing 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi commented May 13, 2022

Uh oh!

Praveen2112 commented May 16, 2022

Uh oh!

findepi commented May 16, 2022

Uh oh!

Praveen2112 commented May 16, 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!

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!

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!

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!

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!

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!

s2lomon commented May 19, 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!

s2lomon commented May 12, 2022 •

edited

Loading