Hive: add table level JVM lock on HiveCatalog commit operation #2547
Conversation
marton-bod
changed the title
| private static synchronized void initTableLevelLockCache(long evictionTimeout) { | ||
| if (commitLockCache == null) { | ||
| commitLockCache = Caffeine.newBuilder() | ||
| .expireAfterAccess(evictionTimeout, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
How does expireAfterAccess work?
I am worried about this sequence:
- C1 commit comes, gets the JVM lock, and HMS lock
- eviction timeout
- C2 commit comes, gets a new JVM lock, and tries to lock
- C1 finishes and unlocks the old JVM lock
There was a problem hiding this comment.
expireAfterAccess is based on the amount of time that elapsed after the last read/write operation on the cache entry. So the scenario that you mentioned could only happen if the C1 commit takes more than the entire eviction timeout period (10 minutes by default) which is very unlikely. Even if it does happen (e.g. due to some extreme lock starvation), and C2 starts the commit operation, it shouldn't have too much of an impact because the HMS locking mechanism will still enforce that there won't be write conflicts between threads (i.e. we would be basically back to where we are in the status quo, without the table-level locks).
There was a problem hiding this comment.
Yeah I like how the worst case scenario in the lock's not working is that we just fall back to the old behavior.
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Thanks so much for this! This is exactly what I was thinking about.
|
Thanks for making this update. Should we add the new config |
|
Thanks @kbendick for reviewing. I also think we probably don't want to include this in the docs in order to avoid overloading users with internally-used configs they'll most likely never need to tune. Tuning this parameter should be very rare and 99% of users would not care about it, I expect. For the remainder of users, they will probably dig into the code anyway and find this flag quite soon after a bit of search, but just my 2 cents. |
|
Let me also take a look today/tomorrow. |
| Optional<Long> lockId = Optional.empty(); | ||
| // getting a process-level lock per table to avoid concurrent commit attempts to the same table from the same | ||
| // JVM process, which would result in unnecessary and costly HMS lock acquisition requests | ||
| ReentrantLock tableLevelMutex = commitLockCache.get(fullName, t -> new ReentrantLock(true)); |
There was a problem hiding this comment.
This seems safe even though one application can interact with multiple metastores as full name includes the catalog name.
aokolnychyi
left a comment
aokolnychyi
left a comment
There was a problem hiding this comment.
This looks good to me. Users could have maintained that lock outside too but I think it won't harm to prevent such cases in the future.
|
Users can only synchronize entire writes not just the commit process. Without this patch, a user has to do the entire write process synchronously to gain the same effect. For example instead of two jobs preparing files at the same time and then synchronously committing, a user would need to do one write and commit before proceeding to the next. |
|
Thanks for fixing this, @marton-bod! Thanks for reviewing, @RussellSpitzer @pvary @kbendick! |
Yeah that makes sense to me. Only a power user would be likely to tune this parameter and they'd probably go into the code / reach out in one of the channels, etc, like you mentioned. 👍 |
|
Thanks everyone for the reviews and @aokolnychyi for the merge! |
- Add blurbs for apache#2565, apache#2583, and apache#2547. - Make formatting consistent.
* Add 0.12.0 release notes pt 2 * Add more blurbs and fix formatting. - Add blurbs for #2565, #2583, and #2547. - Make formatting consistent. * Add blurb for #2613 Hive Vectorized Reader * Reword blurbs for #2565 and #2365 * More changes based on review comments * More updates to the 0.12.0 release notes * Add blurb for #2232 fix parquet row group filters * Add blurb for #2308
5 participants
Following up on #2540.
This commit introduces table-level JVM locks for commit operations to avoid requesting HMS locks unnecessarily when multiple threads are trying to commit to the same table concurrently. The implementation uses a lock cache in order to instantiate any new locks in a thread-safe manner and evict table keys over time to avoid caching table keys indefinitely.
@RussellSpitzer @raptond @pvary @aokolnychyi