Skip to content

HIVE-27944:When HIVE-LLAP reads the ICEBERG table, a deadlock may occur. #4935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BsoBird wants to merge 2 commits into from
Closed

HIVE-27944:When HIVE-LLAP reads the ICEBERG table, a deadlock may occur. #4935

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2616ba
HIVE-27944:When HIVE-LLAP reads the ICEBERG table, a deadlock may occur.
BsoBird Dec 10, 2023
d789c9c
use hashmap
BsoBird Dec 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions ql/src/java/org/apache/hadoop/hive/ql/exec/tez/SplitGrouper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;

import org.apache.hadoop.hive.ql.io.AcidUtils;
import org.apache.hadoop.hive.ql.io.BucketizedHiveInputFormat;
Expand Down Expand Up @@ -71,8 +70,8 @@ public class SplitGrouper {

// 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 =
new ConcurrentHashMap<>();
private final Map<Map<Path, PartitionDesc>, Map<Path, PartitionDesc>> cache =
Copy link
Contributor

@zhangbutao zhangbutao Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using Hive4-beta?
If it is possible, can we add a test?

Copy link
Contributor Author

@BsoBird BsoBird Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@BsoBird BsoBird Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, if you follow the original definition, there is a high probability of deadlocks when multiple threads access the cache at the same time.

Copy link
Contributor Author

@BsoBird BsoBird Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@BsoBird BsoBird Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhangbutao If there's anything else I can provide, please let me know.

Copy link
Member

@deniskuzZ deniskuzZ Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@BsoBird BsoBird Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@deniskuzZ deniskuzZ Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@BsoBird BsoBird Jan 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@BsoBird BsoBird Jan 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

new HashMap<>();

private final TezMapredSplitsGrouper tezGrouper = new TezMapredSplitsGrouper();

Expand Down