deepak-2605 commented on issue #1825: URL: https://github.com/apache/iceberg-go/issues/1825#issuecomment-5302036936
Versions: Go 1.26.3 on windows/amd64, github.com/apache/arrow-go/v18 at v18.7.0. I dug further and found the actual trigger on my end, and it turns out it's not a timing race at all, it's fully deterministic. require.NoError(t, writeErr) was failing on the very first data file, every single run, with: `open c:\Users\...\TestPositionDeletePartitionedFanoutWriterEarlyStopCancelsRecordP760127849\001: is a directory` Here's why. t.TempDir() returns a raw Windows path (that trailing \001 is just Go's own temp-dir sequence suffix, nothing to do with iceberg-go). That raw path gets passed straight into url.Parse() in newWriterFactory/LoadLocationProvider (table/locations.go) with no file:// prefix. Turns out Go's URL parser reads C: as a scheme when you do that, and dumps everything else into .Opaque instead of .Path: `u, _ := url.Parse(`C:\Users\deepa\...\TestXxx...`) // Scheme="c" Opaque="\Users\deepa\...\TestXxx..." Path=""` Every location provider builds its paths through .JoinPath(...), which only ever touches .Path, and url.URL.String() ignores .Path completely once .Opaque is set. So every .JoinPath() call silently does nothing, and NewDataLocation(fileName) just keeps returning the bare warehouse root, no matter what filename or partition you pass it. os.Create then collides with that directory, since it's literally the temp dir itself, and fails every time on Windows. And that failure is exactly what causes the leak you found. It drives RollingDataWriter.stream() into the error-return path, which deregisters itself via CompareAndDelete before abortAll() ever runs, which is the same gap you already spotted. I traced it directly and got this: `deregister ... selfDeregistered=true recordChLenAtDeregister=15 fanoutWorkers.Wait done err="...is a directory" calling abortAll abortAll writerCount=0` So really there are two bugs tangled together here. Your CompareAndDelete/abortAll gap is real and is the actual leak mechanism in both our runs. But there's also this separate url.Parse issue on Windows paths, which is what makes that gap fire on literally every attempt on my end instead of rarely. And it's not just a test problem either, any real user pointing a local or Hadoop-catalog warehouse at a Windows path without a file:// prefix would hit this and silently write every file to the wrong location. Given how different those two things are, would it make sense to split the path bug into its own issue, and narrow this one down to just the CompareAndDelete/abortAll fix? Happy to file it if so and work on both. -- 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]
