dengzhhu653 commented on code in PR #6783:
URL: https://github.com/apache/hive/pull/6783#discussion_r4022560332
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java:
##########
@@ -691,49 +697,65 @@ private static Collection<String>
catalogsToCache(RawStore rs) throws MetaExcept
* @param conf
* @param runOnlyOnce
* @param shouldRunPrewarm
- */ static synchronized void startCacheUpdateService(Configuration conf,
boolean runOnlyOnce,
+ */ static void startCacheUpdateService(Configuration conf, boolean
runOnlyOnce,
boolean shouldRunPrewarm) {
- if (cacheUpdateMaster == null) {
- initBlackListWhiteList(conf);
- if (!MetastoreConf.getBoolVar(conf, ConfVars.HIVE_IN_TEST)) {
- cacheRefreshPeriodMS =
- MetastoreConf.getTimeVar(conf,
ConfVars.CACHED_RAW_STORE_CACHE_UPDATE_FREQUENCY, TimeUnit.MILLISECONDS);
- }
- LOG.info("CachedStore: starting cache update service (run every {} ms)",
cacheRefreshPeriodMS);
- cacheUpdateMaster = Executors.newScheduledThreadPool(1, new
ThreadFactory() {
- @Override public Thread newThread(Runnable r) {
- Thread t = Executors.defaultThreadFactory().newThread(r);
- t.setName("CachedStore-CacheUpdateService: Thread-" + t.getId());
- t.setDaemon(true);
- return t;
+ // Fast path: after first initialization this is a per-worker-thread no-op
(setConf
+ // calls it on every RawStore construction), so skip the lock entirely.
+ if (cacheUpdateMaster != null && !runOnlyOnce) {
+ return;
+ }
+ synchronized (CACHE_UPDATE_SERVICE_LOCK) {
+ if (cacheUpdateMaster == null) {
+ initBlackListWhiteList(conf);
+ if (!MetastoreConf.getBoolVar(conf, ConfVars.HIVE_IN_TEST)) {
+ cacheRefreshPeriodMS =
+ MetastoreConf.getTimeVar(conf,
ConfVars.CACHED_RAW_STORE_CACHE_UPDATE_FREQUENCY, TimeUnit.MILLISECONDS);
+ }
+ LOG.info("CachedStore: starting cache update service (run every {}
ms)", cacheRefreshPeriodMS);
+ cacheUpdateMaster = Executors.newScheduledThreadPool(1, new
ThreadFactory() {
+ @Override public Thread newThread(Runnable r) {
+ Thread t = Executors.defaultThreadFactory().newThread(r);
+ t.setName("CachedStore-CacheUpdateService: Thread-" + t.getId());
+ t.setDaemon(true);
+ return t;
+ }
+ });
+ if (!runOnlyOnce) {
+ cacheUpdateMaster
+ .scheduleAtFixedRate(new CacheUpdateMasterWork(conf,
shouldRunPrewarm), 0, cacheRefreshPeriodMS,
+ TimeUnit.MILLISECONDS);
}
- });
- if (!runOnlyOnce) {
- cacheUpdateMaster
- .scheduleAtFixedRate(new CacheUpdateMasterWork(conf,
shouldRunPrewarm), 0, cacheRefreshPeriodMS,
- TimeUnit.MILLISECONDS);
}
- }
- if (runOnlyOnce) {
- // Some tests control the execution of the background update thread
- cacheUpdateMaster.schedule(new CacheUpdateMasterWork(conf,
shouldRunPrewarm), 0, TimeUnit.MILLISECONDS);
+ if (runOnlyOnce) {
+ // Some tests control the execution of the background update thread
+ cacheUpdateMaster.schedule(new CacheUpdateMasterWork(conf,
shouldRunPrewarm), 0, TimeUnit.MILLISECONDS);
+ }
}
}
- @VisibleForTesting static synchronized boolean stopCacheUpdateService(long
timeout) {
- boolean tasksStoppedBeforeShutdown = false;
- if (cacheUpdateMaster != null) {
- LOG.info("CachedStore: shutting down cache update service");
- try {
- tasksStoppedBeforeShutdown =
cacheUpdateMaster.awaitTermination(timeout, TimeUnit.MILLISECONDS);
- } catch (InterruptedException e) {
- LOG.info("CachedStore: cache update service was interrupted while
waiting for tasks to "
- + "complete before shutting down. Will make a hard stop now.");
+ @VisibleForTesting static boolean stopCacheUpdateService(long timeout) {
+ // The class monitor serializes shutdown with an in-flight prewarm/update
(triggerPreWarm and
+ // triggerUpdateUsingEvent are static synchronized), preserving the
pre-HIVE-30052 semantics:
+ // the executor is not torn down or nulled while its task may still be
mutating the shared
+ // cache. Lock order is class monitor -> CACHE_UPDATE_SERVICE_LOCK; no
path acquires them in
+ // the reverse order. This method is not on the request path, so blocking
here is acceptable.
+ synchronized (CachedStore.class) {
Review Comment:
this acquires two locks for just shutting down the thread, I'm not sure
what's purpose, what if we don't need them?
Also the `static synchronized` methods in this class makes little sense to
me, can we remove them all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
