Skip to content

Commit 2db04bb

Browse files
rcaudyrcaudy
authored
Fix unit test issues introduced with #448 changes, address LTM lock leaks, and fix a few chunk leaks (deephaven#571)
* Clean up some repetitive LTM resets. * Introduce before/after distinction in reset, don't throw error for LTM lock held in "before" mode. Assert lock available, regardless. * Update many unit tests to ensure they have a paired "before" LTM reset with their "after" rest. * Fixed SourceTable mock tests. * Fix definition ordering issue in ParquetTableReadWriteTest. * Fix SparseSelect chunk leak. * Fix rollup chunk leak. * Fix ordering bug in TestLiveness setup. * WIP on fixing codec column tests. * Comment out part of the bad test, and flag for TODO in a new issue.
1 parent 13d0273 commit 2db04bb

File tree

67 files changed

+346
-299
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

67 files changed

+346
-299
lines changed

‎DB-test/src/test/java/io/deephaven/db/util/TestWorkerPythonEnvironment.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ public void setUp() throws Exception {
2929
ProcessEnvironment.basicInteractiveProcessInitialization(Configuration.getInstance(), TestWorkerPythonEnvironment.class.getCanonicalName(), new StreamLoggerImpl(System.out, LogLevel.INFO));
3030
}
3131
LiveTableMonitor.DEFAULT.enableUnitTestMode();
32-
LiveTableMonitor.DEFAULT.resetForUnitTests();
32+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
3333
}
3434

3535
@Override
3636
protected void tearDown() throws Exception {
3737
super.tearDown();
38-
LiveTableMonitor.DEFAULT.resetForUnitTests();
38+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
3939
}
4040

4141
public void testNumpyImport() {

‎DB/src/main/java/io/deephaven/db/tables/ColumnDefinition.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,20 +79,32 @@ public static ColumnDefinition<DBDateTime> ofTime(String name) {
7979

8080
public static <T> ColumnDefinition<T> ofVariableWidthCodec(
8181
String name, Class<T> dataType, String codecName) {
82+
return ofVariableWidthCodec(name, dataType, null, codecName);
83+
}
84+
85+
public static <T> ColumnDefinition<T> ofVariableWidthCodec(
86+
String name, Class<T> dataType, Class<?> componentType, String codecName) {
8287
Objects.requireNonNull(name);
8388
Objects.requireNonNull(dataType);
8489
Objects.requireNonNull(codecName);
8590
ColumnDefinition<T> cd = new ColumnDefinition<>(name, dataType);
91+
cd.setComponentType(componentType);
8692
cd.setObjectCodecClass(codecName);
8793
return cd;
8894
}
8995

9096
public static <T> ColumnDefinition<T> ofFixedWidthCodec(
9197
String name, Class<T> dataType, String codecName, String codecArguments, int width) {
98+
return ofFixedWidthCodec(name, dataType, null, codecName, codecArguments, width);
99+
}
100+
101+
public static <T> ColumnDefinition<T> ofFixedWidthCodec(
102+
String name, Class<T> dataType, Class<?> componentType, String codecName, String codecArguments, int width) {
92103
Objects.requireNonNull(name);
93104
Objects.requireNonNull(dataType);
94105
Objects.requireNonNull(codecName);
95106
ColumnDefinition<T> cd = new ColumnDefinition<>(name, dataType);
107+
cd.setComponentType(componentType);
96108
cd.setObjectCodecClass(codecName);
97109
if (codecArguments != null) {
98110
cd.setObjectCodecArguments(codecArguments);

‎DB/src/main/java/io/deephaven/db/tables/live/LiveTableMonitor.java

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -590,22 +590,29 @@ public void resetCycleTime() {
590590
* {@link DynamicNode#setRefreshing(boolean) refreshing}. Additionally {@link #start()} may not be called.</p>
591591
*/
592592
public void enableUnitTestMode() {
593+
if (unitTestMode) {
594+
return;
595+
}
593596
if (!allowUnitTestMode) {
594597
throw new IllegalStateException("LiveTableMonitor.allowUnitTestMode=false");
595598
}
596599
if (refreshThread.isAlive()) {
597600
throw new IllegalStateException("LiveTableMonitor.refreshThread is executing!");
598601
}
602+
assertLockAvailable("enabling unit test mode");
603+
unitTestMode = true;
604+
unitTestModeHolder = ExceptionUtils.getStackTrace(new Exception());
605+
unitTestRefreshThreadPool = makeUnitTestRefreshExecutor();
606+
}
607+
608+
private void assertLockAvailable(@NotNull final String action) {
599609
if (!LiveTableMonitor.DEFAULT.exclusiveLock().tryLock()) {
600-
log.error().append("Lock is held when enabling unit test mode, with previous holder: ").append(unitTestModeHolder).endl();
610+
log.error().append("Lock is held when ").append(action).append(", with previous holder: ").append(unitTestModeHolder).endl();
601611
ThreadDump.threadDump(System.err);
602612
LiveTableMonitorLock.DebugAwareFunctionalLock lock = (LiveTableMonitorLock.DebugAwareFunctionalLock) LiveTableMonitor.DEFAULT.exclusiveLock();
603-
throw new IllegalStateException("Lock is held when enabling unit test mode, with previous holder: " + lock.getDebugMessage());
613+
throw new IllegalStateException("Lock is held when " + action + ", with previous holder: " + lock.getDebugMessage());
604614
}
605615
LiveTableMonitor.DEFAULT.exclusiveLock().unlock();
606-
unitTestMode = true;
607-
unitTestModeHolder = ExceptionUtils.getStackTrace(new Exception());
608-
unitTestRefreshThreadPool = Executors.newFixedThreadPool(1, new UnitTestRefreshThreadFactory());
609616
}
610617

611618
/**
@@ -857,13 +864,16 @@ public void requestRefresh(@NotNull final LiveTable liveTable) {
857864

858865
/**
859866
* Clear all monitored tables and enqueued notifications to support {@link #enableUnitTestMode() unit-tests}.
867+
* @param after Whether this is *after* a unit test completed, and hence whether held locks should result in an exception
860868
*/
861869
@TestUseOnly
862-
public void resetForUnitTests() {
863-
resetForUnitTests(false, 0, 0, 0, 0);
870+
public void resetForUnitTests(final boolean after) {
871+
resetForUnitTests(after, false, 0, 0, 0, 0);
864872
}
865873

866-
public void resetForUnitTests(boolean randomizedNotifications, int seed, int maxRandomizedThreadCount, int notificationStartDelay, int notificationAdditionDelay) {
874+
public void resetForUnitTests(boolean after,
875+
final boolean randomizedNotifications, final int seed, final int maxRandomizedThreadCount,
876+
final int notificationStartDelay, final int notificationAdditionDelay) {
867877
final List<String> errors = new ArrayList<>();
868878
this.notificationRandomizer = new Random(seed);
869879
this.notificationAdditionDelay = notificationAdditionDelay;
@@ -912,13 +922,17 @@ public void resetForUnitTests(boolean randomizedNotifications, int seed, int max
912922
} catch (InterruptedException e) {
913923
errors.add("Interrupted while trying to cleanup jobs in unit test refresh thread pool");
914924
}
915-
unitTestRefreshThreadPool = Executors.newFixedThreadPool(1, new UnitTestRefreshThreadFactory());
925+
unitTestRefreshThreadPool = makeUnitTestRefreshExecutor();
916926

917927
if (!errors.isEmpty()) {
918928
final String message = "LTM reset for unit tests reported errors:\n\t" + String.join("\n\t", errors);
919929
System.err.println(message);
920-
throw new IllegalStateException(message);
930+
if (after) {
931+
throw new IllegalStateException(message);
932+
}
921933
}
934+
935+
assertLockAvailable("resetting for unit tests");
922936
}
923937

924938
/**
@@ -1828,6 +1842,10 @@ private void ensureUnlocked(@Nullable final List<String> errors) {
18281842
}
18291843
}
18301844

1845+
private ExecutorService makeUnitTestRefreshExecutor() {
1846+
return Executors.newFixedThreadPool(1, new UnitTestRefreshThreadFactory());
1847+
}
1848+
18311849
@TestUseOnly
18321850
private class UnitTestRefreshThreadFactory extends NamingThreadFactory {
18331851

‎DB/src/main/java/io/deephaven/db/v2/SparseSelect.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,8 +313,9 @@ private static void doShiftSingle(SafeCloseablePair<Index, Index> shifts, Sparse
313313
final ChunkSource.FillContext [] fcs = new ChunkSource.FillContext[outputSources.length];
314314
final WritableChunkSink.FillFromContext [] ffcs = new WritableChunkSink.FillFromContext[outputSources.length];
315315

316-
try (final SafeCloseableArray<ChunkSource.FillContext> ignored = new SafeCloseableArray<>(fcs);
317-
final SafeCloseableArray<WritableChunkSink.FillFromContext> ignored2 = new SafeCloseableArray<>(ffcs);
316+
try (final SafeCloseableArray<WritableChunk<Values>> ignored = new SafeCloseableArray<>(values);
317+
final SafeCloseableArray<ChunkSource.FillContext> ignored2 = new SafeCloseableArray<>(fcs);
318+
final SafeCloseableArray<WritableChunkSink.FillFromContext> ignored3 = new SafeCloseableArray<>(ffcs);
318319
final SharedContext sharedContext = SharedContext.makeSharedContext();
319320
final OrderedKeys.Iterator preIt = shifts.first.getOrderedKeysIterator();
320321
final OrderedKeys.Iterator postIt = shifts.second.getOrderedKeysIterator()) {

‎DB/src/main/java/io/deephaven/db/v2/by/ssmcountdistinct/BucketSsmDistinctRollupContext.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ public BucketSsmDistinctRollupContext(ChunkType chunkType, int size) {
2525
public void close() {
2626
super.close();
2727
lengthCopy.close();
28+
countCopy.close();
29+
countResettable.close();
30+
starts.close();
2831
valueResettable.close();
2932
ssmsToMaybeClear.close();
3033
}

‎DB/src/main/java/io/deephaven/db/v2/parquet/ParquetTableWriter.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,11 @@ public Integer get() {
451451
columnWriter.addVectorPage(bufferToWrite, repeatCount, transferObject.rowCount(), nullValue);
452452
repeatCount.clear();
453453
} else if (supportNulls) {
454-
columnWriter.addPage(bufferToWrite, nullValue, transferObject.rowCount());
454+
try {
455+
columnWriter.addPage(bufferToWrite, nullValue, transferObject.rowCount());
456+
} catch (Exception e) {
457+
throw e;
458+
}
455459
} else {
456460
columnWriter.addPageNoNulls(bufferToWrite, transferObject.rowCount());
457461
}

‎DB/src/main/java/io/deephaven/db/v2/parquet/TypeInfos.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ private static Optional<TypeInfo> lookupTypeInfo(ColumnSource column) {
5959
}
6060

6161
private static TypeInfo lookupTypeInfo(ColumnDefinition column) {
62-
if (column.getComponentType() != null) {
62+
if (column.getComponentType() != null && column.getObjectCodecType() == ColumnDefinition.ObjectCodecType.DEFAULT) {
6363
return lookupTypeInfo(column.getComponentType()).orElseGet(() -> new CodecType());
6464
}
6565
if (StringSet.class.isAssignableFrom(column.getDataType())) {

‎DB/src/test/java/io/deephaven/db/tables/utils/TestTableManagementTools.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
8888
@Before
8989
public void setUp() {
9090
LiveTableMonitor.DEFAULT.enableUnitTestMode();
91-
LiveTableMonitor.DEFAULT.resetForUnitTests();
91+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
9292

9393
testRootFile.mkdirs();
9494
}
@@ -111,7 +111,7 @@ public void tearDown() {
111111
TestCase.assertTrue(success);
112112
}
113113
} finally {
114-
LiveTableMonitor.DEFAULT.resetForUnitTests();
114+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
115115
}
116116
}
117117

‎DB/src/test/java/io/deephaven/db/tables/utils/TestTableTools.java

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ public void setUp() throws Exception {
6969
oldCheckLtm = LiveTableMonitor.DEFAULT.setCheckTableOperations(false);
7070
oldLogEnabled = CompilerTools.setLogEnabled(true);
7171
LiveTableMonitor.DEFAULT.enableUnitTestMode();
72+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
7273
UpdatePerformanceTracker.getInstance().enableUnitTestMode();
7374

7475
scope = new LivenessScope();
@@ -105,22 +106,25 @@ public void tearDown() throws Exception {
105106
scope.release();
106107
CompilerTools.setLogEnabled(oldLogEnabled);
107108
LiveTableMonitor.DEFAULT.setCheckTableOperations(oldCheckLtm);
108-
LiveTableMonitor.DEFAULT.resetForUnitTests();
109109
AsyncClientErrorNotifier.setReporter(oldReporter);
110110

111-
if (TEST_ROOT_FILE.exists()) {
112-
int tries = 0;
113-
boolean success = false;
114-
do {
115-
try {
116-
FileUtils.deleteRecursively(TEST_ROOT_FILE);
117-
success = true;
118-
} catch (Exception e) {
119-
System.gc();
120-
tries++;
121-
}
122-
} while (!success && tries < 10);
123-
TestCase.assertTrue(success);
111+
try {
112+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
113+
} finally {
114+
if (TEST_ROOT_FILE.exists()) {
115+
int tries = 0;
116+
boolean success = false;
117+
do {
118+
try {
119+
FileUtils.deleteRecursively(TEST_ROOT_FILE);
120+
success = true;
121+
} catch (Exception e) {
122+
System.gc();
123+
tries++;
124+
}
125+
} while (!success && tries < 10);
126+
TestCase.assertTrue(success);
127+
}
124128
}
125129
}
126130

‎DB/src/test/java/io/deephaven/db/util/liveness/TestLiveness.java

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@
55
import io.deephaven.db.tables.utils.TableTools;
66
import io.deephaven.db.v2.TstUtils;
77
import junit.framework.TestCase;
8+
import org.junit.After;
9+
import org.junit.Before;
810
import org.junit.Test;
9-
import org.openjdk.jmh.annotations.Setup;
10-
import org.openjdk.jmh.annotations.TearDown;
1111

1212
/**
1313
* Unit tests for liveness code.
@@ -17,22 +17,25 @@ public class TestLiveness extends TestCase {
1717
private boolean oldCheckLtm;
1818
private LivenessScope scope;
1919

20-
@Setup
20+
@Before
21+
@Override
2122
public void setUp() throws Exception {
2223
super.setUp();
23-
scope = new LivenessScope();
24-
LivenessScopeStack.push(scope);
2524
LiveTableMonitor.DEFAULT.enableUnitTestMode();
25+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
2626
oldCheckLtm = LiveTableMonitor.DEFAULT.setCheckTableOperations(false);
27+
scope = new LivenessScope();
28+
LivenessScopeStack.push(scope);
2729
}
2830

29-
@TearDown
31+
@After
32+
@Override
3033
public void tearDown() throws Exception {
3134
super.tearDown();
3235
LivenessScopeStack.pop(scope);
3336
scope.release();
3437
LiveTableMonitor.DEFAULT.setCheckTableOperations(oldCheckLtm);
35-
LiveTableMonitor.DEFAULT.resetForUnitTests();
38+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
3639
}
3740

3841
@SuppressWarnings("JUnit4AnnotatedMethodInJUnit3TestCase")

‎DB/src/test/java/io/deephaven/db/v2/FuzzerTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ public void testLargeSetOfFuzzerQueriesSimTime() throws IOException, Interrupted
208208
final long seed1 = DBDateTime.now().getNanos();
209209
for (long iteration = 0; iteration < 5; ++iteration) {
210210
for (int segment = 0; segment < 10; segment++) {
211-
LiveTableMonitor.DEFAULT.resetForUnitTests();
211+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
212212
try (final SafeCloseable ignored = LivenessScopeStack.open()) {
213213
System.out.println("// Segment: " + segment);
214214
final int firstRun = segment * 10;

‎DB/src/test/java/io/deephaven/db/v2/LiveTableTestCase.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ abstract public class LiveTableTestCase extends BaseArrayTestCase implements Upd
3737
protected void setUp() throws Exception {
3838
super.setUp();
3939
LiveTableMonitor.DEFAULT.enableUnitTestMode();
40-
LiveTableMonitor.DEFAULT.resetForUnitTests();
40+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
4141
SystemicObjectTracker.markThreadSystemic();
4242
oldMemoize = QueryTable.setMemoizeResults(false);
4343
oldReporter = AsyncClientErrorNotifier.setReporter(this);
@@ -47,7 +47,7 @@ protected void setUp() throws Exception {
4747
@Override
4848
protected void tearDown() throws Exception {
4949
super.tearDown();
50-
LiveTableMonitor.DEFAULT.resetForUnitTests();
50+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
5151
QueryTable.setMemoizeResults(oldMemoize);
5252
AsyncClientErrorNotifier.setReporter(oldReporter);
5353
}

‎DB/src/test/java/io/deephaven/db/v2/MultiColumnSortTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ public class MultiColumnSortTest {
2626
@Before
2727
public void setUp() {
2828
LiveTableMonitor.DEFAULT.enableUnitTestMode();
29-
LiveTableMonitor.DEFAULT.resetForUnitTests();
29+
LiveTableMonitor.DEFAULT.resetForUnitTests(false);
3030
}
3131

3232
@After
3333
public void teardown() {
34-
LiveTableMonitor.DEFAULT.resetForUnitTests();
34+
LiveTableMonitor.DEFAULT.resetForUnitTests(true);
3535
}
3636

3737
@Test

‎DB/src/test/java/io/deephaven/db/v2/QueryTableCrossJoinTestBase.java

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,23 +29,12 @@
2929
import static io.deephaven.db.v2.TstUtils.*;
3030

3131
public abstract class QueryTableCrossJoinTestBase extends QueryTableTestBase {
32+
3233
private final int numRightBitsToReserve;
3334
public QueryTableCrossJoinTestBase(int numRightBitsToReserve) {
3435
this.numRightBitsToReserve = numRightBitsToReserve;
3536
}
3637

37-
@Override
38-
protected void setUp() throws Exception {
39-
super.setUp();
40-
ChunkPoolReleaseTracking.enableStrict();
41-
}
42-
43-
@Override
44-
protected void tearDown() throws Exception {
45-
super.tearDown();
46-
ChunkPoolReleaseTracking.checkAndDisable();
47-
}
48-
4938
private TstUtils.ColumnInfo[] getIncrementalColumnInfo(final String prefix, int numGroups) {
5039
String[] names = new String[]{"Sym", "IntCol"};
5140

‎DB/src/test/java/io/deephaven/db/v2/QueryTableNaturalJoinTest.java

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,6 @@
3333
import static java.util.Arrays.asList;
3434

3535
public class QueryTableNaturalJoinTest extends QueryTableTestBase {
36-
@Override
37-
protected void setUp() throws Exception {
38-
super.setUp();
39-
ChunkPoolReleaseTracking.enableStrict();
40-
}
41-
42-
@Override
43-
protected void tearDown() throws Exception {
44-
super.tearDown();
45-
ChunkPoolReleaseTracking.checkAndDisable();
46-
}
4736

4837
public void testNaturalJoinRehash() {
4938
setExpectError(false);

‎DB/src/test/java/io/deephaven/db/v2/QueryTableSortTest.java

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,6 @@
3030
import static io.deephaven.db.v2.TstUtils.*;
3131

3232
public class QueryTableSortTest extends QueryTableTestBase {
33-
@Override
34-
protected void setUp() throws Exception {
35-
super.setUp();
36-
ChunkPoolReleaseTracking.enableStrict();
37-
}
38-
39-
@Override
40-
protected void tearDown() throws Exception {
41-
ChunkPoolReleaseTracking.checkAndDisable();
42-
super.tearDown();
43-
}
4433

4534
public void testSort() {
4635
final Table result0 = newTable(c("Unsorted", 3.0, null, 2.0), c("DataToSort", "c", "a", "b"));

0 commit comments

Comments
 (0)