CASSANDRA-16939: Only remove shutdown hook for fatal heap OOMs#4691
CASSANDRA-16939: Only remove shutdown hook for fatal heap OOMs#4691Shanzita wants to merge 1 commit intoapache:trunkfrom
Conversation
| drainField.set(StorageService.instance, testHook); | ||
|
|
||
| // "Direct buffer memory" is a non-fatal OOM — the shutdown hook must be preserved | ||
| try |
There was a problem hiding this comment.
@Shanzita please rework this to more idiomatic org.assertj.core.api.Assertions.assertThatThrownBy which is use extensively through the code base and is more succinct / readable.
There was a problem hiding this comment.
Thank you so much for your reviews! I have removed the try/catch/fail block to use assertThatThrownBy as suggested.
| } | ||
|
|
||
| @Test | ||
| public void testShutdownHookNotRemovedForNonFatalOom() throws Exception |
There was a problem hiding this comment.
these two test methods are 95% same. All it differs on is message in that exception and assertion if it should remove shutdown hook or not. Could you do a helper method which encapsulates it all with appropriate parameters to pass to it so the actual test methods will be effectively one-liners?
You could also make FORCE_HEAP_OOM_IGNORE_SET public and take its first value to test on a real value which will be used in production. If we ever change it in JVMStabilityInspector but not here we will not be testing anything "real". You should ideally iterate over all values, not sure how feasible it is.
There was a problem hiding this comment.
Thank you so much for your reviews! I have extracted a helper method, made FORCE_HEAP_OOM_IGNORE_SET public, and iterating over all values in the fatal OOM test.
for fatal heap OOMs
Shanzita
force-pushed
the
CASSANDRA-16939-trunk
branch
from
84be9a8 to
e79db48
Compare
| } | ||
|
|
||
| private static final Set<String> FORCE_HEAP_OOM_IGNORE_SET = ImmutableSet.of("Java heap space", "GC Overhead limit exceeded"); | ||
| public static final Set<String> FORCE_HEAP_OOM_IGNORE_SET = ImmutableSet.of("Java heap space", "GC Overhead limit exceeded"); |
There was a problem hiding this comment.
@Shanzita annotate this with @VisibleForTesting. You might additionally make it package protected as both test and this class are in the same package.
2 participants
Description:
CASSANDRA-16939: Only remove shutdown hook for fatal heap
OOMs
Previously,
StorageService.removeShutdownHook()wascalled unconditionally
for all
OutOfMemoryErrors inJVMStabilityInspector.inspectThrowable().This removed the drain shutdown hook even for non-fatal
OOMs like
"Direct buffer memory" or "unable to create new native
thread", preventing
graceful shutdown afterward even though the JVM was
otherwise healthy.
The fix guards
removeShutdownHook()with a newisHeapSpaceOom()methodthat reuses the existing
FORCE_HEAP_OOM_IGNORE_SETtoonly remove the hook
for fatal heap OOMs ("Java heap space", "GC Overhead limit
exceeded").
Changes:
JVMStabilityInspector.java: GuardremoveShutdownHook()withisHeapSpaceOom()checkJVMStabilityInspectorTest.java: Added tests verifyingshutdown hook is preserved
for non-fatal OOMs and removed for fatal OOMs
Tests:
patch by Shanizta Siddiqua; reviewed by for
CASSANDRA-16939