HIVE-29762: Synchronization logic added as a part of HIVE-24428 is having hash collision issue - #6651
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. |
|
|
Check the context of HIVE-24428, the Metastore server won't delete the created directory upon dynamic partitions from different clients collide, and this seems to be a in-process lock, do we really need it to coordinate clients? |
Thanks for the review @dengzhhu653. As per my understanding, here partition folder in the storage is getting created and they maintain Map<PartValEqWrapperLite, Boolean> to store whether the partition folder is successfully created or not (value attribute in the map). In case of any exception while adding the partition, here the directory is getting cleaned based on the boolean value present in the map for that particular partition. There will be below two scenarios,
Please let me know whether my understanding is correct or I am missing something. Thanks. |
I see
We still have the same issue in this case, image A and B client add the same partition |
dengzhhu653
left a comment
dengzhhu653
left a comment
There was a problem hiding this comment.
This mitigates the hash collision somehow, while we should come up with a better way to coordinate different clients
@dengzhhu653 I raised a followup JIRA (HIVE-29789) to fix the issue related to multiple HMS instances involved. |
6 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.