yashmayya opened a new pull request, #19304: URL: https://github.com/apache/pinot/pull/19304
Follow-up to #19166. `ColocatedJoinEmptyPartitionTest`, added there, fails intermittently in `setUp` ([example run](https://github.com/apache/pinot/actions/runs/32197636088/job/95904769015?pr=19292)): ``` ColocatedJoinEmptyPartitionTest>CustomDataQueryClusterIntegrationTest.setUp:117->waitForAllDocsLoaded:140 Caught exception while checking the condition, error message: Failed to load 6 documents into table: ColocatedJoinEmptyPartitionLeft ``` ## Cause The test deleted the segment tar files it had already uploaded. `ClusterTest#uploadSegments` picks its push mode at random. Half the time it pushes only the metadata and records a `file://` download URI that points at the tar file itself: ```java "file://" + segmentTarFile.getParentFile().getAbsolutePath() + "/" + URIUtils.encode(segmentTarFile.getName()) ``` The tar file is then the only deep store copy, and the servers read it after the upload call has returned. The test sets up two tables, and both used the same segment and tar directories. Each call emptied them first: 1. The left table uploads. Deep store now points into the shared tar directory. 2. The right table's setup empties that directory, which deletes the left table's only deep store copy. 3. The servers fetch, find nothing, and retry 3 times. The segments stay in `ERROR`. 4. The count query never reaches 6 rows. Two things had to line up, which is why it failed rarely: the random push mode had to select metadata, and the servers had to still be fetching. A loaded machine makes the second one likely. ## Fix Each table gets its own segment and tar directory, so no table empties a directory another one uploaded from. Separate directories also keep `uploadSegments`, which pushes every tar it finds, from reading the other table's segments. That was the only reason to empty the directory before. A new assertion after both tables are set up makes sure that the tar files of both are still on disk. The original defect fails that assertion directly, instead of as a load timeout 60 seconds later. ## Verification - The failure reproduces on the code before this change once the push mode is forced to metadata and the directory is emptied directly after the first upload, which is the timing a loaded CI machine produces. Same signature as CI: `AttemptsExceededException` out of `downloadSegmentFromDeepStore`, and load failures for the left table only. - The new assertion fails on the code before this change with no forced push mode and no timing change. - With the fix, the test passes with the push mode forced to metadata, and with the random mode. ## Not addressed here The same shape exists in shared test infrastructure: `BaseClusterIntegrationTest#createAndUploadSegmentFromFile` empties the shared directories on every call, and `SSBQueryTest` empties them inside a per-table loop. Those survive because each waits for documents to load before the next upload, and their tables have one replica. This test had neither: it uploads two tables back to back with `numReplicas=2`. Making the shared upload path keep its own deep store copy would remove the hazard for every test, and belongs in its own change. -- 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]
