peter-toth commented on code in PR #59009:
URL: https://github.com/apache/spark/pull/59009#discussion_r4093507166
##########
core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java:
##########
@@ -1109,30 +1171,50 @@ public int getNumDataPages() {
}
/**
- * Returns the underline long[] of longArray.
+ * Returns the underlying long array. If the pointer-array allocation in a
previous
+ * {@link #reset()} failed, this method retries that allocation before
returning.
+ *
+ * @throws SparkOutOfMemoryError if the pointer array still cannot be
allocated
*/
public LongArray getArray() {
+ restoreArrayAfterFailedReset();
assert(longArray != null);
return longArray;
}
/**
- * Reset this map to initialized state.
+ * Reset this map to initialized state. If {@link #free()} has been called,
the map cannot be
+ * reinitialized and this method is a no-op. If the replacement pointer
array cannot be
+ * allocated, the map remains empty and the next lookup, {@link
#getArray()}, or
+ * {@link #maxNumKeysIndex()} retries the allocation.
+ *
+ * @throws SparkOutOfMemoryError if the replacement pointer array cannot be
allocated
*/
- public void reset() {
+ public synchronized void reset() {
+ if (freed) {
+ return;
+ }
Review Comment:
**Finding 8.** The description says destructive iteration stays terminal,
but only `restoreArrayAfterFailedReset` checks `destructiveIterator`, and only
when a recovery is pending. `reset()` never checks it. After
`destructiveIterator()`, a `reset()` that allocates successfully leaves
`longArrayRecoveryRequired == false`, so the gate never runs and the map is
fully usable again.
I checked it on this head: `destructiveIterator()`, then `reset()`,
`lookup()` and `append()` all succeed. On master the same `reset()` failed with
an NPE in `freeArray(null)`. The new `longArray != null` check below is what
lets it through now.
It also leaves `destructiveIterator` set. From reading, a `spill()`
triggered by another consumer would then still go to the old iterator, which
spills and frees the reused map's data pages.
Failing the same way as the recovery gate closes it:
```suggestion
if (freed) {
return;
}
if (destructiveIterator != null) {
throw new IllegalStateException(
"BytesToBytesMap cannot be used after destructiveIterator() has been
called");
}
```
And a test for it:
```java
@Test
public void resetAfterDestructiveIterationFails() {
memoryManager.limit(PAGE_SIZE_BYTES);
BytesToBytesMap map =
new BytesToBytesMap(taskMemoryManager, blockManager,
serializerManager, 256, 0.5, 4000);
try {
map.destructiveIterator();
assertThrows(IllegalStateException.class, map::reset);
assertEquals(0L, taskMemoryManager.getMemoryConsumptionForThisTask());
} finally {
map.free();
}
}
```
I ran it both ways. It passes with the check above (56 tests green across
both suites). Without the check it fails with `Expected
java.lang.IllegalStateException to be thrown, but nothing was thrown`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
