JAMES-4156 refactoring Blobstore deleted message vault for appending messages into single bucket #2902
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /**************************************************************** | ||
| * Licensed to the Apache Software Foundation (ASF) under one * | ||
| * or more contributor license agreements. See the NOTICE file * | ||
| * distributed with this work for additional information * | ||
| * regarding copyright ownership. The ASF licenses this file * | ||
| * to you under the Apache License, Version 2.0 (the * | ||
| * "License"); you may not use this file except in compliance * | ||
| * with the License. You may obtain a copy of the License at * | ||
| * * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 * | ||
| * * | ||
| * Unless required by applicable law or agreed to in writing, * | ||
| * software distributed under the License is distributed on an * | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * | ||
| * KIND, either express or implied. See the License for the * | ||
| * specific language governing permissions and limitations * | ||
| * under the License. * | ||
| ****************************************************************/ | ||
|
|
||
| package org.apache.james.vault.blob; | ||
|
|
||
| import java.time.Clock; | ||
| import java.time.ZonedDateTime; | ||
| import java.util.UUID; | ||
|
|
||
| import org.apache.commons.lang3.RandomStringUtils; | ||
| import org.apache.james.blob.api.BlobId; | ||
| import org.apache.james.blob.api.PlainBlobId; | ||
|
|
||
| public class BlobIdTimeGenerator { | ||
|
Contributor
There was a problem hiding this comment. I don't understand why the whole PR goes around the BlobId.Factory design 🤷
Contributor
There was a problem hiding this comment. Please re read the interface. BlobId.Factory is about *parsing* BlobId This work is about *generating* them. Maybe I am missing something, despite investing several hours on the topic. Maybe a hand on example could help?
Contributor
There was a problem hiding this comment. Maybe I completely miss the point of BlobId.Factory. My understanding is that the Factory is responsible for creating instances of BlobId that point to a specific object (generate BlobIds), either by wrapping/decorating In this case you are encoding custom paths as a PlainBlobId bypassing the factory which is not consistent with how things are done in the rest of the codebase. I was kinda expecting a TimePrefixedBlobIdFactory (don't focus on this specific name) which would encapsulate the path manipulation. I may misunderstand by your path manipulation appears to be very similar to what is done in
Contributor
There was a problem hiding this comment. Hello @jeantil
I see no generate method. Generation is an actual responsibility of the BlobStore, or of its caller. Proof: At the minimum a concrete clarification would be a Please note that in the example taken: The decision to rely on a MailKey as the storage identifier is not ecoded in the BlobId.Factory either.
TBH as we are not actively using the date information encoded in the object name this present little added value to operate of a "typed" blobId. The only use of information encoded by the blobId to a useful process is to my knowledge the GC to leverage a grace period cf Also my consern about "typed" BlobId is that it interoperate badly. The beauty of the proposed design here (PlainBloId factory) is that we naturally handle historical data without needing some complex fallback logic for an information that is not actually needed. The So how do we deal with this:
Cheers. |
||
| public static final String BLOB_ID_GENERATING_FORMAT = "%d/%02d/%s/%s/%s"; | ||
|
|
||
| static BlobId currentBlobId(Clock clock) { | ||
| ZonedDateTime now = ZonedDateTime.now(clock); | ||
| int month = now.getMonthValue(); | ||
| int year = now.getYear(); | ||
| String randomizerA = RandomStringUtils.insecure().nextAlphanumeric(1); | ||
| String randomizerB = RandomStringUtils.insecure().nextAlphanumeric(1); | ||
|
|
||
| return new PlainBlobId(String.format(BLOB_ID_GENERATING_FORMAT, year, month, randomizerA, randomizerB, UUID.randomUUID())); | ||
| } | ||
| } | ||
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.
Will this allow nesting below root ?
Imagine I can or want to have a single s3 bucket for all james related stuff. My bucket is
free-hosting-foo-23402how do I configure the Vault so that deleted messages are nested under
free-hosting-foo-23402/james-deleted-message-vault/xxxxso I can have the mailqueue and other s3 backed store write in other subdirectories ?
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.
No the bucket is (by default)
james-deleted-message-vaultHere it is not a subfolder of the main S3 bucket.
The goal of this work is not to get a single bucket
But operate distributed james server off 3 buckets (default, james-uploads, james-deleted-message-vault)
(So as an Ops I need to create 3 buckets and not one each month)
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.
I think using the term Bucket or bucket name for the blobstore was a mistake. it overlaps in a bad way with the s3 notions.
Here you are manipulating a blobstore bucket.
I think this can be stored as a subpath of a single underlying S3 bucket by using the prefix property on the S3 configuration
This is all very confusing (to me at least) as this prefix only allows to shift the whole namespace in which case it is impossible to split storage of some use cases among different bucket
On a more direct musing I think the previous design influences the naming too much here, the term
singlewill become confusing quite fast. I think I would use DEFAULT_VAULT_BUCKET insteadThere 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.
+1
But as said CF previous refactoring that may not be something easy that we are willing to change.
I don't want this to mess up with mailbox GC. I'd rather have a separate blob store bucket if given the choice.
Also a separate blob store bucket makes loads of sense as the storage constraints are not the standard james ones (glacier?)
Cf comment above.
Uh oh!
There was an error while loading. Please reload this page.
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.
I thought we agreed that this work for now just allows going from multiple S3 buckets to just one for the vault only, but it does not put all blobs under the root bucket.
The goal in the end would be to put all blobs under one S3 bucket yes. But that's not what this PR intends to do. That would require more changes and discussions.
Is it still unclear on that point @jeantil ?
Uh oh!
There was an error while loading. Please reload this page.
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.
no that's perfectly clear
This can be achieved today using the
prefixornamespaceconfiguration of blob.properties (sorry I don't exactly remember which I had to use, this is not so well documented) but is not really my point.I think the constant should not contain the word
SINGLEas this only makes sense in the current refactoring context. if you were building this feature from scratch today based on the new design you would not call it single.As I understand it this is the
Blobstorebucket name (so a logical bucket name) under which all the blobs of the feature implemented by the plugin will be written. The actual S3 bucket name in which this logical bucket is stored can be different based on theblob.propertiesconfiguration ( prefix and namespace will affect the actual storage namespace)Hence my proposal to call it
DEFAULT_VAULT_BUCKET(by no means definitive)I'm not sure how my proposed name change or how using the prefix/namespace features of blob.properties changes would afffect the mailbox GC ... I propose we discuss this with actual examples on the ADR. This goes beyond my intention for the PR.