HIVE-27944:When HIVE-LLAP reads the ICEBERG table, a deadlock may occur. #4935
Conversation
Since the Properties object implement HashTable interface, all the methods of the HashTable interface are synchronised. In a multi-threaded environment, a deadlock will occur when propA.equals(propB) and propB.equals(propA) occur at the same time.
|
@deniskuzZ @abstractdog Hi.Could you help review this PR? |
| // TODO This needs to be looked at. Map of Map to Map... Made concurrent for now since split generation | ||
| // can happen in parallel. | ||
| private static final Map<Map<Path, PartitionDesc>, Map<Path, PartitionDesc>> cache = | ||
| private final Map<Map<Path, PartitionDesc>, Map<Path, PartitionDesc>> cache = |
There was a problem hiding this comment.
Are you using Hive4-beta?
If it is possible, can we add a test?
There was a problem hiding this comment.
I've looked through the code, and the way I've done it doesn't change any of the logical behaviour of the code. I just defined the cache object as a non-static property to avoid the scenario of multiple threads calling it at the same time.
There was a problem hiding this comment.
Furthermore, if you follow the original definition, there is a high probability of deadlocks when multiple threads access the cache at the same time.
There was a problem hiding this comment.
Since the nature of the problem is that HashTable causes deadlocks in concurrent scenarios, I'm not sure what kind of test case would be appropriate to add ....
We can only conclude that whenever a cache object is accessed concurrently, it will trigger a deadlock almost immediately.
For details on deadlocks, you can check jira, which has deadlock-related logging information.
There was a problem hiding this comment.
@zhangbutao If there's anything else I can provide, please let me know.
There was a problem hiding this comment.
add the SQL you've used to repro the concurrency issue into that q.file
note, in case of iceberg, place the q file under itests/qtest-iceberg
mvn test -Dtest=TestIcebergLlapLocalCliDriver -Dqfile=mytest.q -Drat.skip=true -Dtest.output.overwrite=true
There was a problem hiding this comment.
@deniskuzZ Hi. @deniskuzZ . My current problem is that I can't reproduce the problem consistently. I haven't worked out a way to reproduce the problem consistently yet. So I have no way to add a q.file test case at the moment.
There was a problem hiding this comment.
@BsoBird, could you please share the repro steps that led to the issue? it doesn't have to be consistent. I just want to create at least one test covering that part of the code. At this moment we have 0 coverage.
There was a problem hiding this comment.
@deniskuzZ Just Read IcebergTable. and using hive llap. Before I used this patch, this problem occurred occasionally. After I used this patch, the problem disappeared. However, I'm not currently using a hashMap as a replacement for ConcurrentHashMap.
There was a problem hiding this comment.
@deniskuzZ If it is difficult to override this change with SQL, can we re-run the previous SQL to test the LLAP+ICEBERG reads and writes? Because our change must affect all the SQL that reads and writes to the ICEBERG table, we just need to verify that the output is the same as before.
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
1
…---Original---
From: ***@***.***>
Date: Fri, Mar 15, 2024 08:19 AM
To: ***@***.***>;
Cc: "PLASH ***@***.******@***.***>;
Subject: Re: [apache/hive] HIVE-27944:When HIVE-LLAP reads the ICEBERG table,a deadlock may occur. (PR #4935)
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the ***@***.*** list if the patch is in need of reviews.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
1 |
|
I can't just approve this without the test. I wanted to repro the issue locally and then create a q test, but I didn't have time. |
|
Sir.I'm having a hard time triggering this bug directly via SQL because as of now, I'm not sure how to consistently reproduce the problem. in our production env, after we added the patch.The problem disappeared, so I never looked at how to reproduce the bug. Can we test this class in code without using SQL? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
4 participants
What changes were proposed in this pull request?
Modified the modifier of the cache property in the SplitGroup class to make it non-static.
JIRA: https://issues.apache.org/jira/browse/HIVE-27944
Why are the changes needed?
Since the Properties object implement HashTable interface, all the methods of the HashTable interface are synchronised.
In a multi-threaded environment, a deadlock will occur when propA.equals(propB) and propB.equals(propA) occur at the same time.
The problem can be solved with minimal modification by changing SplitGroup.cache to a non-static property.
Does this PR introduce any user-facing change?
no
Is the change a dependency upgrade?
no
How was this patch tested?
no need. Because this patch doesn't change the execution logic of any code