-
Notifications
You must be signed in to change notification settings - Fork 116
Remove full file load from publish step #3724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove full file load from publish step #3724
Conversation
This updates the logic in the `publishMavenCentralBundle` task so that it no longer loads the full contents of the file into memory. It does this by maintaining a buffer and then writing the contents of the buffer into the stream, which allows us to avoid materializing the full element into memory. This is to try and fix a Java heap memory error that we saw previously (see: https://github.com/FoundationDB/fdb-record-layer/actions/runs/19108664941/job/54605649248). While I was here, I also updated the boundary for the HTTP multi-part form from the current time millis to a UUID, in response to a comment from the previous PR (FoundationDB#3723 (comment)).
build.gradle
Outdated
| // Stream the file in chunks to avoid loading it all into memory | ||
| bundleFile.withInputStream { input -> | ||
| connection.outputStream << input | ||
| byte[] buffer = new byte[8192] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into it, I think the << does this already.... https://github.com/apache/groovy/blob/GROOVY_3_0_22/src/main/java/org/codehaus/groovy/runtime/IOGroovyMethods.java#L206
I think the actual problem is that if you are not streaming, HTTPUrlConnection will use a PosterOutputStream which is ByteArrayOutputStream.
I think you need to call setChunkedStreamingMode(int chunklen) or setFixedLengthStreamingMode(long contentLength)
I think we should be able to calculate the content length if you first convert the header and footer to byte[].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sure. I can give that a shot
This reverts commit f161cbc.
This reverts #3723 and #3724 and returns us back to publishing via the nexus publishing API. The impetus for this is that the last approach got stuck in a publishing state after the upload (see: https://github.com/FoundationDB/fdb-record-layer/actions/runs/19117047380/job/54633288001, which succeeded, but for which we didn't get artifacts actually published despite the server upload succeeding). When we are in this mode, we expect the build to fail with a 400 error letting us know that we didn't upload anything. However, if we didn't `close` the staging repositories (which is the step that is failing), then legacy API would not be expected to upload anything to the central repo. So we need to continue to do that part even though we think it will fail.
This updates the logic in the
publishMavenCentralBundletask so that it no longer loads the full contents of the file into memory. It does this by maintaining a buffer and then writing the contents of the buffer into the stream, which allows us to avoid materializing the full element into memory. This is to try and fix a Java heap memory error that we saw previously (see: https://github.com/FoundationDB/fdb-record-layer/actions/runs/19108664941/job/54605649248).While I was here, I also updated the boundary for the HTTP multi-part form from the current time millis to a UUID, in response to a comment from the previous PR (#3723 (comment)).