Repository navigation
JAMES-4231 S3 Object Compaction - #3193
HesandaLiyanage wants to merge 8 commits into
Conversation
|
Hi @HesandaLiyanage ! First of all thank you very much for trying to solve this. However 6k lines changes in one commit is too much for code readability and reviewing properly. Can you cut this work down to smaller step commits? Thank you :) |
|
And test errors are related to this PR I believe, please have a look too : https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-3193/1/testReport/ |
56e7fb9 to
71d5234
Compare
|
Hi @Arsnael, Thank you very much for the review and guidance! 1. CI Test Failures FixedThe failures in
2. Broken Down Into 6 Logical Step CommitsI have restructured the work from a single monolithic commit into 6 focused, incremental commits:
Please let me know if you would like any further adjustments! |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Hello,
Thanks a lot for this initial contribution that is going overall in the good direction.
I did do a quick initial review of this work, and have already significant structural comments that I would like to see addressed.
Also I believe we would need to have more integration tests:
Given X, Y Z 200 mail population, compaction yield x, y, z object layout afterward, and we matches integrity constraints (no metadata -> 404). We would need that to be performed on ~5 different layouts, probably on top of memory blob store for decent testing time.
We would also need tests that account for failures updating references and ensure that we remain coherent.
I to not think I saw mailbox/cassandra ObjectStorageNotFoundException upon reading blobs from imapUidTable / messageIdTable - this detail is important for correctness as these table can create entry with past generation blobs (COPY / MOVE)
We are missing /docs documentation for this.
Wouldn't you mind addressing those feedbacks?
|
hello, The structure looks better yes thank you @HesandaLiyanage https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-3193/2/ New test failures still related to this work. It seems some guice bindings are missing:
Requested by: Learn more:
Requested by: Learn more: 2 errors |
|
All requested changes and feedbacks have been addressed and pushed as individual atomic commits: 1. Structural & Algorithmic Improvements
2. Guice Dependencies in Non-Cassandra Environments
3. Cassandra Mailbox Mappers Fallback
4. Coherence Under Failure Integration Tests
5. Layout Matrix Integration Tests (200 Mails)
6. Documentation
All changes are completed and tested. Thank you both for the detailed review! |
|
Thanks for all the fixes. That's a major changeset. I really woupd support (out of the CI) Property Based Testing for this so that we can develop confidence on it. ( https://www.scalatest.org/user_guide/property_based_testing for instance ) it should be able to generate large set of object within a generation, perform compact and delete in random manner under various profiles (delete heavy or not - configurable and tested accross a wide range of value, possibly randomly set between each compacts) and ensure along several runs that 1- count of object decrease in a predicable fashion and 2- no dead references are created. The review is massive, and I do not want to delegate this to an AI, I'll try to cover a review of this by the end of the week. Cheers, Benoit |
Thanks so much, really appreciate you taking the time on this. Yeah let's do the property based testing. that's way better than me just eyeballing it after a few manual runs. i'll start working around with that on my side in the meantime. looking forward to your feedback by end of week, no rush though given how big this changeset is. |
|
@HesandaLiyanage I see you being a student in your github profile. Would you be interested in a Google Summer of Code with the ASF James project ? If this is meaningful for you we could keep going on the S3 compaction stuff and plan eventually S3 GC / compaction implemented as Apache Spark jobs (for instance). I think the timing is not bad for 2027, though I recognize I do not have the precise schedule in mind. |
Yes, absolutely! I would love to participate in Google Summer of Code with James project. Taking on S3 compaction and exploring large-scale S3 GC/compaction with Apache Spark sounds like a fantastic project, and I'd be honored to work on it with you and the community. |
I'm glad to hear that. This discussion cannot stay solely on github. I propose you to introduce yourself on the DEV mailing list, quote your objectives, link this PR, and potentially say that you would eventually be interested by a GSOC proposed by Benoit to continue the work started on S3 compaction task but make it more scalable with Spark. |
Sounds good! I'll introduce myself on the dev mailing list and keep working on the S3 compaction for now. Thank you so much for the opportunity . |
|
https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-3193/3/testReport/ Related errors on CassandraBlobIdRepairerIntegrationTest. Please help review and fix @HesandaLiyanage :) |
|
Which S3 should be used now? SeaweedFS? Silo? RustFS? Garage? ofc i dont want get my data corrupted. |
That's a good question. We had a try at triaging alternatives to MinIO earlier this year, as you can see here: https://issues.apache.org/jira/browse/JAMES-4167 It wasn't very conclusive back then unfortunately. But maybe now it's worth taking an other look if you want a nice little extra challenge ;) Specially now that MinIO removed all their public images it seems... I cans ee RustFS for example now released in a v1.0.0 not long ago. Maybe that means they are finally on a more stable and mature version of their product. It would be worth getting an other shot at it IMO |
i tested with James RustFS and for now its bad, about 60-70% compability (used warp to test) also not even close to Silo (MinIO fork 99-100%) and SeaweedFS (which is about 85% combability) perfomance (but yeah much lower ram usage) I tesing RustFS 1.0.0 version, both original and from sources. But you right... there NO real options for now, for standalone single server setup (postgres-app) i think need focus on RustFS combability, for clusters (distribution-app) CephFS i think? Because SeaweedFS dont have full claster support in opensources version, Garage much worse than Seaweed in any way (compability/perfomance, latency) and not working on Windows (but i DO custom version and make it WORK on windows from sourses and even THIS show better than RustFS...) So what you think? |
|
Also why only S3 or PostgreSQL? Why not filesystem like in Stalwart and Dovecot? |
Done!. |
so now not James itself makes compression - now it doings S3 storage? |
No, James itself still does all the compression. S3 is just an object store that receives and stores the bytes James sends it.
|
well i looking forward to move this task to RustFS S3 for example which can compress by myself with zstd/lz4, just as for me the more mail server dont do others jobs the better it doing main job - mail exchanging. |
yeah fair enough, and compression in james isn't forced either way, if the underlying s3 engine you're using already handles it at the storage layer, ceph or rustfs, you can just turn it off on the james side with blobstore.compression.enabled=false and let the backend do its thing. but for plain cloud s3 like aws, wasabi, backblaze b2 etc, none of those do any packing or compression on their own, so having james handle it out of the box still matters a lot there, otherwise your put costs and storage footprint climb pretty fast. |
yeah customizations always wins! so yeah for cloud its sure userful, also only zstd for now? i think in some cases lz4 needed (lower latency). And deduplication methods its like in ZFS? Just Windows Post-processing dedup wins for now. |
Good questions, though i think this is kinda drifting outside the scope of this specific PR, lz4 vs zstd tradeoffs and dedup approaches would probably get a lot more visibility on the dev mailing list or as a github discussion rather than buried in a PR thread. Really appreciate you thinking through this stuff though, i think its better if we keep this thread focused on the compaction review and the CI fixes for now |
We used to have a poorly performing file maildir backend. We deprecated it: bad performance, directory traversal, no maintainer, no known deployment.
If options exist to tell s3 to compress and do the job that's nice. I am not aware TBH and we could add support for this in S3BlobStoreDAO ZstdBlobStoreDAO should work out of the box atop file implem. If this is not the case a separated PR is welcomed.
Bad choice, it won't compress base64 as it doesn't do entropy coding. Most benefit of compression is the base64 overhead on large attachment, which is capped at ~33%.
If it is standard (ie amazon s3 supports this) then it is welcomed in S3BlobStoreDAO
Yes precisely. Also bear in mind that one can implement out-of-band-compression, and solely compress old 3+ month old mail as a tiering strategy... I do not know if RustFS for instance is able to automate that with a S3 policy.
Event worse if one operate a on prem S3 compatible system like Ceph Rados Gateway.
Lz4 is not fit cf earlier entropy coding point. Zstd had decent java port. More niche algorithm would need to have good perf java implems. I confess I do not have in mind viable java alternative, but with hashing I did bench SHA-256 vs BLAKE3 and fornow JVM implenm of BLAKE3 is worse than SHA-256. I'd be careful with exotic implem and have their viability backed with a JMH micro benchmark.
You are free to drop james dedup and experiment with FS level dedup. My take on it is that FS do not have all the infos James have and would be doing more work to achieve partial results. I'm expecting it not to deduplicate as well as James can do but I do not have solid comparison to answer you and the question indeed is interesting.
Completly, but this is a nice discussion! Cheers, Benoit |
I confirm this is the right way to go! |
While I 100% subscribe to the point as a James integrator, |
|
thanks for laying all that out, @chibenwa , appreciated. for file/postgres, since ZstdBlobStoreDAO is just a decorator, getting it properly wired and tested on top of FileBlobStoreDAO and PostgresBlobStoreDAO seems like its own clean follow up PR too. I can work on that |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
One hour review...
I did not finished reviewing BlobCompactionAlgorithm - it is not conform to the design - I thus stopped.
There's already a lot to fix.
Please ask explicitly for a review (reviewer feature on Github) if needed.
Cheers.
Benoit (tired)
thanks benoit, appreciate you pushing through this even while tired. cheers, |
|
Hello @chibenwa , I’ve completed several rounds of reviews and polished the pr more. |
|
@chibenwa Ping |
|
Can we have a cleanup of the git commits into something more understandable for a clear history of the PR implem? It would help following for a better review as well |
fe76012 to
a5a5386
Compare
Hello !! |
a5a5386 to
90b2707
Compare
…ator guides for S3 Object Compaction
…r, and BlobIdUpdater SPI
…O, and compaction tasks
…allback reconciler
90b2707 to
d012e72
Compare
|
Hello @HesandaLiyanage Thanks for your work. I'll try to commit to do a review this weekend. Cheers, Benoit |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Kid woke up I did not go to the heart of it
(BlobCompactionAlgoritm)
We are clearly improving in terms of quality but this change set is way to big to get though as is. And 100+ comments is too much to keep tracking efficiently.
I would advocate to split it in subparts that each make sense. Can you please propose on server-dev@james.apache.org mailing list or on the JIRA ticket a list of small/smaller PRs ?
We can keep this PR open as a draft / a reference.
I'd have a reference branch JAMES-4231 on the repo to handle this as a eature branch.
Best regards,
Benoit
| or server-side encryption (SSE-S3 / SSE-KMS / SSE-C); it is incompatible with client-side | ||
| `AESBlobStoreDAO` encryption because random byte ranges in AES ciphertext cannot be decrypted | ||
| in isolation. If server-side encryption with customer-provided keys (SSE-C) is used, ensure |
There was a problem hiding this comment.
This is false.
AES cannot be appled after chunking but can be applied before.
It is the order of composition of the Blob stores that matters.
Indeed AES needs to be applied before (each blob independently encrypted)
| To compact standalone blobs of a completed generation into chunk objects: | ||
|
|
||
| .... | ||
| curl -XDELETE "http://ip:port/blobs?action=initial-compaction&generation=1" |
There was a problem hiding this comment.
The verb "DELETE" looks weird, how about "POST" ?
| } | ||
|
|
||
| private final CassandraAsyncExecutor cassandraAsyncExecutor; | ||
| private final Map<BlobId, Set<UUID>> references; |
There was a problem hiding this comment.
This won't fit in memory.
We could envision a separated table IMO to store this...
| .then(); | ||
| } | ||
|
|
||
| private Mono<Void> updateMessageReferences(UUID messageUuid, String oldIdStr, String newIdStr) { |
There was a problem hiding this comment.
This method is too complex and deserves to be extracted in small parts
| .then(); | ||
| } | ||
|
|
||
| private Mono<Void> updateMessageReferences(UUID messageUuid, String oldIdStr, String newIdStr) { |
There was a problem hiding this comment.
This method is too complex and deserves to be extracted in small parts
| * A chunk consists of sequential slot payloads followed by a trailing footer: | ||
| * [FORMAT_BYTE (1B)] [SLOT_1] [SLOT_2] ... [SLOT_N] [FOOTER_TEXT] [FOOTER_LEN (4B)] [FOOTER_POS (8B)] | ||
| */ | ||
| public class ChunkFormat { |
There was a problem hiding this comment.
Long complicated methods in here too. Code extraction needed...
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o instanceof InitialBlobCompactionTask that) { | ||
| return Objects.equals(request, that.request); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(request); | ||
| } |
There was a problem hiding this comment.
Do we need equals and hashcode for tasks ?
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (o instanceof GCBlobCompactionTask that) { | ||
| return Objects.equals(request, that.request); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(request); | ||
| } |
|
|
||
| @Provides | ||
| @Singleton | ||
| public Optional<BlobCompactionAlgorithm> optionalBlobCompactionAlgorithm(Injector injector) { |
There was a problem hiding this comment.
Pass the wanted fields as parameter instead of injector
| return new BlobCompactionTask(algorithm, buildCompactionRequest(request), clock); | ||
| } | ||
|
|
||
| private CompactionRequest buildCompactionRequest(Request request) { |
Hey @chibenwa , Yes agree, 8k+ lines in one PR is way too much to follow. Im switching #3193 to draft so it stays around as the reference implementation. im going to split it into 8 smaller sub PRs, most under 300 lines, with the core engine in its own module. i'm putting the breakdown on server-dev and the JIRA ticket now so you can check it before i start. once the JAMES-4231 feature branch exists on the repo i'll open the sub PRs against it in order. Thanks for the guidance !!! |
|
Based on the feedback, I propose splitting the work into these incremental PRs:
Does this breakdown and ordering look good? Would it be possible to create a clean JAMES-4231 feature branch in apache/james-project, based on master, for the sub-PRs to target, with a final PR from that branch to master? The current JAMES-4231 branch contains the full implementation, so it can remain the reference but doesn’t provide a shared upstream base. |
With little refinement yes it looks great to me. |
Thank you. I'll start working on it |
4 participants
JIRA: https://issues.apache.org/jira/browse/JAMES-4231
Context & Problem
High email ingest volumes in Apache James deployments utilizing S3/MinIO object storage result in hundreds of millions of small S3 objects (average body/header size ~20–50KB). Operating at this granularity introduces high S3 API request fees (PUT/GET/LIST), elevated bucket listing latency during storage maintenance and garbage collection, and reduced overall object storage throughput.
Solution Overview
This PR implements autonomous S3 object compaction for Apache James, packing historical standalone blobs into large, immutable chunk files (~100MB target size) with slot-level virtual addressing and transparent single-request HTTP ranged read support.
Key Components & Architecture
Chunk Format (
ChunkFormat):[Header (16B)][Slot 0]...[Slot N-1][Footer][Footer Size (4B)][End Magic 0x454F4643]content-encoding=zstd\n), allowing single-slot ranged reads without decompressing or buffering the full 100MB chunk.readRange(..., -65536, -1)) fetches chunk slot metadata insrc/adr/0076-s3-object-compaction.md.Virtual Slot Addressing & Ranged Reads (
ChunkedBlobStoreDAO&ChunkId):<family>_<generation>_chunk<hash>~<offset>~<limit>.ChunkedBlobStoreDAOtransparently intercepts slot references and issues HTTP byte-range requests directly against the underlying storage connector (readRange(bucket, chunkId, offset, offset + limit - 1)).ChunkMarkerregex (^\d+_\d+_chunk[A-Za-z0-9_-]{16,}(~\d+~\d+)?$) to ensure bloom-filter GC never misclassifies chunks as unreferenced garbage.Compaction Pipeline & Crash Safety (
BlobCompactionAlgorithm):initialCompact):DEFAULT_CANDIDATE_BATCH_SIZE = 1000) so candidate byte arrays are packed and freed window-by-window without buffering the generation's payloads in heap.messageV3,messageIdToImapUid,messageIdTable).gcCompact):CassandraBlobIdRepairer):blobReferenceMappingand restores Cassandra references. Also heals any transient inconsistency window across Cassandra tables.Configuration Guards & Scope:
DELETE /blobs?scope=compaction&generation=<gen>&family=<fam>.Memory Characteristics & Operational Ceiling:
Verification & Tests
server/blob/blob-compaction: 47 tests passed (100%), including allocation-bound proof test and task serialization.server/blob/blob-storage-strategy: 78 tests passed (100%).mailbox/cassandra(CassandraBlobId*IntegrationTest): 3/3 passed with real Cassandra 5.0.9 testcontainers.server/blob/blob-s3: Ranged read and MinIO S3 end-to-end compaction integration tests passed.server/container/guice/distributed: 43 tests passed (100%).