CachingHiveMetastore: refactor cache creation and flushCache() #4872
Conversation
| return loadPartitionsByNames(partitionNames); | ||
| } | ||
| }, executor)); | ||
| class CacheFactory |
There was a problem hiding this comment.
mixture of factory method and method object patterns
| tableStatisticsCache.invalidateAll(); | ||
| partitionStatisticsCache.invalidateAll(); | ||
| rolesCache.invalidateAll(); | ||
| caches.forEach(Cache::invalidateAll); |
There was a problem hiding this comment.
it's nothing new, it's very similar to Closer already used in many places, the difference is that here objects can be 'closed' many times
There was a problem hiding this comment.
Nit: We prefer normal for-each loops. Use foreach if there's a specific benefit to doing so, such as the terminal operation of a stream. Note that Map.forEach is far superior to for-each as it avoids unpacking the Entry.
| @@ -114,6 +116,7 @@ | |||
| private final LoadingCache<HivePrincipal, Set<RoleGrant>> roleGrantsCache; | |||
| private final LoadingCache<String, Set<RoleGrant>> grantedPrincipalsCache; | |||
| private final LoadingCache<String, Optional<String>> configValuesCache; | |||
| private final List<LoadingCache<?, ?>> caches = new ArrayList<>(); | |||
There was a problem hiding this comment.
Initialize in constructor to ImmutableList
|
Benefit is: all caches are flushed (previously 4 were missing), code is
more readable, avoids errors that somebody forgot to call caches.add
wt., 18 sie 2020, 15:30 użytkownik Łukasz Osipiuk <notifications@github.com>
napisał:
… ***@***.**** commented on this pull request.
loop in flushCache LGTM. I do not see a benefit from the rest of this PR.
------------------------------
In
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java
<#4872 (comment)>:
> @@ -114,6 +116,7 @@
private final LoadingCache<HivePrincipal, Set<RoleGrant>> roleGrantsCache;
private final LoadingCache<String, Set<RoleGrant>> grantedPrincipalsCache;
private final LoadingCache<String, Optional<String>> configValuesCache;
+ private final List<LoadingCache<?, ?>> caches = new ArrayList<>();
Initialize in constructor to ImmutableList
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4872 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGAYE7XXH23GIFUFCENVD3SBJ67PANCNFSM4QDMRVTA>
.
|
Fine :) THanks |
|
For the commit message, we write the title like a phrase or sentence, rather than prefixing with a component. So something like
|
| return loadPartitionsByNames(partitionNames); | ||
| } | ||
| }, executor)); | ||
| class CacheFactory |
There was a problem hiding this comment.
please extract private static inner class. Class definition inside the method body is hard to follow.
There was a problem hiding this comment.
If it was deeper, it would be a good candidate for moving "up"; everything not too nested (let's say up to 4-5 nested { / }) is not a readability problem on today's wide screens; it even improves readability (moving this "up" would mean some boilerplate like explicit field and constructor declarations).
I think it's possible to configure Checkstyle such way: allow up to some 5 nested {s, block more than that.
There was a problem hiding this comment.
in intellij it's possible to configure in Inspections > Class metrics / Method metrics
| @@ -13,6 +13,8 @@ | |||
| */ | |||
There was a problem hiding this comment.
WRT to commit message. It looks like the refactor is not the most important change here. The most important part is that you added missing invalidate for caches. And refactor is only about preventing this to happen in future again. Please consider writing better commit message.
There was a problem hiding this comment.
couldn't find a better message: it's not just a bugfix, but also refactoring to remove duplication
There was a problem hiding this comment.
it's not just a bugfix, but also refactoring to remove duplication
So maybe two commits. One bugfix. Second refactofing. WDYT?
|
👋 @iirekm - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
No description provided.