-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-30274][Core] Avoid BytesToBytesMap lookup hang forever when holding keys reaching max capacity #26914
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.
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
Conversation
|
Test build #115418 has finished for PR 26914 at commit
|
|
We have a job that seems related to this issue, this job sometimes hangs in a BroadcastJoin stage(task 131726) for about 2 hours. |
|
Thank you so much for making the validation example in the PR description! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM.
This is logically correct. And, since it's difficult to add a logic into safeLookup, this looks like the best location to prevent this bug. I also verified with the given example with MAX_CAPACITY = (1 << 21) in both master/2.4.
Merged to master/2.4
…lding keys reaching max capacity
### What changes were proposed in this pull request?
We should not append keys to BytesToBytesMap to be its max capacity.
### Why are the changes needed?
BytesToBytesMap.append allows to append keys until the number of keys reaches MAX_CAPACITY. But once the the pointer array in the map holds MAX_CAPACITY keys, next time call of lookup will hang forever.
### Does this PR introduce any user-facing change?
No
### How was this patch tested?
Manually test by:
```java
Test
public void testCapacity() {
TestMemoryManager memoryManager2 =
new TestMemoryManager(
new SparkConf()
.set(package$.MODULE$.MEMORY_OFFHEAP_ENABLED(), true)
.set(package$.MODULE$.MEMORY_OFFHEAP_SIZE(), 25600 * 1024 * 1024L)
.set(package$.MODULE$.SHUFFLE_SPILL_COMPRESS(), false)
.set(package$.MODULE$.SHUFFLE_COMPRESS(), false));
TaskMemoryManager taskMemoryManager2 = new TaskMemoryManager(memoryManager2, 0);
final long pageSizeBytes = 8000000 + 8; // 8 bytes for end-of-page marker
final BytesToBytesMap map = new BytesToBytesMap(taskMemoryManager2, 1024, pageSizeBytes);
try {
for (long i = 0; i < BytesToBytesMap.MAX_CAPACITY + 1; i++) {
final long[] value = new long[]{i};
boolean succeed = map.lookup(value, Platform.LONG_ARRAY_OFFSET, 8).append(
value,
Platform.LONG_ARRAY_OFFSET,
8,
value,
Platform.LONG_ARRAY_OFFSET,
8);
}
map.free();
} finally {
map.free();
}
}
```
Once the map was appended to 536870912 keys (MAX_CAPACITY), the next lookup will hang.
Closes #26914 from viirya/fix-bytemap2.
Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit b2baaa2)
Signed-off-by: Dongjoon Hyun <[email protected]>
|
Thank you so much, @viirya ! |
|
Thank you! @dongjoon-hyun |
What changes were proposed in this pull request?
We should not append keys to BytesToBytesMap to be its max capacity.
Why are the changes needed?
BytesToBytesMap.append allows to append keys until the number of keys reaches MAX_CAPACITY. But once the the pointer array in the map holds MAX_CAPACITY keys, next time call of lookup will hang forever.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Manually test by:
Once the map was appended to 536870912 keys (MAX_CAPACITY), the next lookup will hang.