Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4449: Revisit table locking pattern in the catalog ......................................................................
Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/5710/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 184: private static final long TIMEOUT_FOR_TBL_LOCK_IN_MSEC = 3600000; > an hour? Done Line 184: private static final long TIMEOUT_FOR_TBL_LOCK_IN_MSEC = 3600000; > We usually use the MS suffix elsewhere, i.e., Renamed it. How much higher? :) Line 187: * Tries to acquire a table lock while holding the catalogLock_. Returns true if it > Tries to acquire the catalogLock_ and the lock of 'tbl' in that order. Done Line 189: * false otherwise. > point out which locks are held on return in both scenarios. Done Line 191: public boolean tryToAcquireCatalogLocks(Table tbl) { > how about something briefer like tryLockTable? Hm, that's what I had in the beginning. Wanted to indicate that we also acquire the catalogLock in this function. Anyways, I renamed it. LMK if that works better. Line 205: Thread.yield(); > comment on significance of this. i'm assuming you need it to prevent spinni Done Line 205: Thread.yield(); > I believe this will call sched_yield() on Linux, so may still end up with b Sleep for how long? Line 321: tbl.getLock().lock(); > does it make sense to have rw locks for tables as well? sounds like that mi Done Line 321: tbl.getLock().lock(); > never mind, this is only catalogd logic. Done Line 975: tbl.getLock().unlock(); > where do we unlock the catalog write-lock? In L959. http://gerrit.cloudera.org:8080/#/c/5710/2/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 71: // Lock protecting this table > explain why this is needed (why do we reacquire the lock when we already ha Hm, what do you mean by "reacquire the lock when we already have it"? Which case do your refer to that this occurs? Line 72: private final ReentrantLock tableLock_ = new ReentrantLock(true); > Thinking about this a little more: Why does this lock need to be fair? We d We need a fair lock because you may have relatively short operations on a table blocked forever by long running operations such as refresh on the same table. The retry logic doesn't guarantee that each operation will make progress eventually. http://gerrit.cloudera.org:8080/#/c/5710/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 173: * ensures that the catalog lock is never held for a long period of time, preventing > this pattern only applies to single-table update operations, worth pointing Good point. Updated the comment. Line 3127: for (org.apache.hadoop.hive.metastore.api.Partition part: > single line Done -- To view, visit http://gerrit.cloudera.org:8080/5710 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id08e21da31deb1f003b3cada4517651f3b3b2bb2 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
