xiangfu0 opened a new pull request, #20016: URL: https://github.com/apache/pinot/pull/20016
`TarCompressionUtilsTest.testLz4ArchiveRoundTrip` (added in #19946) is flaky. In the `LZ4_FAST` case it builds an explicit `LZ4_FAST` archive and a default `.tar.lz4` archive from the same file, then requires them to be byte-identical. It has failed in Pinot Unit Test Set 1 on #19986 and #19995, for example: ``` TarCompressionUtilsTest.testLz4ArchiveRoundTrip:131 arrays don't have the same size. expected [3163524] but found [3163522] ``` ### Cause `TarCompressionUtils` creates entries with `new TarArchiveEntry(file, entryName)`, which reads the file's access and status-change times. Because the output stream uses `BIGNUMBER_STAR`, commons-compress writes those times into the xstar header fields in whole seconds (no PAX headers are involved). Reading the file to build the first archive moves its access time forward, so the second archive's header differs whenever a second boundary falls between creating the file and that first read. To confirm this, I uncompressed both tars and compared their headers. Only the atime field (offset 486) and the header checksum differ. Adding a 1.1 s sleep after writing the input file makes the original assertion fail every time. ### Change (test-only) The test now decodes the default archive's own tar stream, re-encodes it with `LZ4_FAST`, and requires the result to match the archive byte for byte. This still checks the default `.tar.lz4` compressor end to end, through `createCompressedTarFile(inputFile, defaultArchive)`, but no longer depends on file metadata. Other approaches I rejected: - Comparing the decompressed tar streams doesn't help: the headers themselves differ. - Setting fixed file times before each archive doesn't help either: setting times updates ctime, which is also recorded. To check that the new assertion still catches a wrong default, I temporarily mapped `.tar.lz4` to `LZ4_HC` and then to commons `LZ4_FRAMED`. The test failed in both cases. Production behavior is unchanged. Recording atime/ctime makes archives of the same input non-reproducible, and Pinot never restores those times on untar. Nothing depends on reproducible archive bytes, though, so dropping them is left out of this PR. ### Testing - `./mvnw -pl pinot-common -am -Dtest=TarCompressionUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test`: passed 5 consecutive runs, plus a run with the 1.1 s sleep added. - pinot-common: `spotless:apply`, `license:format`, `checkstyle:check`, `license:check` and a warning-enabled `test-compile` are all clean. -- 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]
