feat: Replace JClouds with AWS SDK + File NIO [DHIS2-20648]#23918
Conversation
david-mackessy
changed the title
jbee
left a comment
jbee
left a comment
There was a problem hiding this comment.
Some details to sort out:
- enum name change (migration needed?)
- details store API error contract (null vs exception, which exception, consistency)
Philip-Larsen-Donnelly
left a comment
Philip-Larsen-Donnelly
left a comment
There was a problem hiding this comment.
The slow speed of the S3 uploads* makes this painful to test, but apart from that I didn't find any obvious issues with the storage.
Tested apps, icons, avatars and data values.
*Note - everything is slow. Even the icon fix patch too a couple of minutes to populate icons and the API call timed out after one minute.
This testing was extremely valuable @Philip-Larsen-Donnelly, thank you. How features use the BlobStore is very ad-hoc. When issues are known (like bundled app install or icon patch), these can be looked at & improved 👍 |
| * ZipFile.getInputStream(zipEntry)} used by app install) the wrapper throws. | ||
| * | ||
| * <p>If the caller's stream is non-resetable we pre-buffer it into a byte array and use {@link | ||
| * RequestBody#fromBytes}, which is safely re-readable. Resetable streams (file streams, byte |
There was a problem hiding this comment.
After digging a bit (After some tips from Claude); I don't think file stream (FileInputStream) is actually resettable, as it doesn't implement the mark and reset methods. Not a critical thing per se, but it makes the next point more relevant:
If the stream is not resettable, we read the stream into a buffer, effectively reading the whole file into our heap. We might want to have a hard limit somewhere, either based on heap space or just a hard limit on individual file sizes; Cause now there's a way to upload files big enough to run us OOM, either with one big file, or with several, simultaneous medium files.
However, looking at "normal" use and expectations; And the fact that the user can set a sane file size limit in dhis.conf; I don't see this issue as a blocker, rather something to keep in mind; Maybe add to documentation?
There was a problem hiding this comment.
We can incorporate the system max file size here as a safeguard?
max.file_upload_size - default is 10MB.
At least then we don't interfere with system restrictions, which is probably what we want.
|
4 participants
AI assisted 🤖
Summary
Replaces the JClouds object storage implementation. This was primarily planned due to the archiving of the JCLouds library. Replacing it has resulted in many positives:
BlobStoreServiceinterfaceFileStoreProvidertypesFileStoreProviderJClouds tried to handle all variants of
FileStoreProvider(noted in the table below) but it did so in an inefficient way:FileStoreProviderto decide what to doFileStoreProvidertypesFileStoreProvidertype without affecting othersNew implementations
Rename
JCloudsprefix withBlobStoreAppStorageSource.JCLOUDS->AppStorageSource.BLOB_STORERemoved
JCloudsStoreimplementationAdded
Implementations for:
S3BlobStoreServiceFileSystemBlobStoreServiceTransientBlobStoreServiceFix
As part of the replacement, recursive deletes now operate correctly. Previous issue of some files being left behind depending on provider type. Haven't seen any evidence of this while testing.
Testing
Automated
Manual
Tested providers (
s3,filesystem,transient) with the scenarios:PATCH /api/icons-> icons installed2g_negativeindex.htmlfrom custom app