HIVE-29762: Synchronization logic added as a part of HIVE-24428 is having hash collision issue - #6651
HIVE-29762: Synchronization logic added as a part of HIVE-24428 is having hash collision issue#6651mdayakar wants to merge 1 commit into
Conversation
| @@ -2017,8 +2007,10 @@ public enum ConfVars { | |||
| + "e.g. javax.net.ssl.trustStore=/tmp/truststore,javax.net.ssl.trustStorePassword=pwd.\n " + | |||
| "If both this and the metastore.dbaccess.ssl.* properties are set, then the latter properties \n" + | |||
| "will overwrite what was set in the deprecated property."), | |||
| METASTORE_NUM_STRIPED_TABLE_LOCKS("metastore.num.striped.table.locks", "hive.metastore.num.striped.table.locks", 32, | |||
| "Number of striped locks available to provide exclusive operation support for critical table operations like add_partitions."), | |||
| METASTORE_NUM_STRIPED_TABLE_LOCKS("metastore.num.striped.table.locks", "hive.metastore.num.striped.table.locks", 65536, | |||
There was a problem hiding this comment.
any particular reason why 65536 was chosen?
There was a problem hiding this comment.
There is no reason, I feel 65536(2^16) is not too high and not too low value and minimizes the hash collisions. If required we can increase this value.
thomasrebele
left a comment
thomasrebele
left a comment
There was a problem hiding this comment.
Thank you for the PR! I've read the ticket description in HIVE-29762, and it's not clear to me whether the concurrency issue is a performance bug or a deadlock problem. If the issue is a performance bug, then the PR looks good to me as-is. If the issue is a deadlock, then the PR would just make the deadlock less likely, making it more difficult to debug in the future. If there's a deadlock, another ticket should be created to find a permanent fix. Could you clarify this in the ticket description, please?
| // lazyWeakLock: lock objects are allocated on first use and held via weak references, | ||
| // so unused stripes are GC'd. A large stripe count keeps hash collisions between | ||
| // unrelated (db, tbl) keys without paying permanent memory cost. | ||
| tablelocks = Striped.lazyWeakLock(numTableLocks); |
…ving hash collision issue.
061fed4 to
08a447b
Compare
@thomasrebele thanks for the review. Its a performance issue, here for the unrelated table names(keys) like a.AA and b.BB for example, it was giving same lock object due to hash collision issue because of that one thread is is waiting to lock and other thread got the lock and working on adding partition. But here partition is getting added to different tables so here both threads can work concurrently. This issue got fixed as a part of current fix. |
|
5 participants
HIVE-29762: Synchronization logic added as a part of HIVE-24428 is having hash collision issue
What changes were proposed in this pull request?
Here hash collisions are happening for unrelated(db.tablename) keys with stripe size of 32 so increased the stripe size to 65536 which will increase memory footprint as 65536 lock objects will get created upfront. To reduce the memory footprint, used Striped.lazyWeakLock() API in which lock objects are lazily allocated and weakly referenced.
Why are the changes needed?
To remove/minimize the hash collisions for the unrelated db.tablename keys.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing testcases.