devmadhuu commented on code in PR #11229:
URL: https://github.com/apache/ozone/pull/11229#discussion_r4185119115
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Consts.java:
##########
@@ -131,6 +131,11 @@ public final class S3Consts {
/** Request header carrying the list of object attributes to return. */
public static final String OBJECT_ATTRIBUTES_HEADER =
"x-amz-object-attributes";
+ // S3 storage class values passed via the `x-amz-storage-class` request
header.
+ public static final String S3_STORAGE_CLASS_STANDARD = "STANDARD";
+ public static final String S3_STORAGE_CLASS_STANDARD_IA = "STANDARD_IA";
+ public static final String S3_STORAGE_CLASS_GLACIER = "GLACIER";
+
Review Comment:
Thanks everyone. Summarising where this landed and proposing a concrete
shape.
Agreement so far: one enum, one vocabulary (@greenwich), reverse lookup must
use both replication and storage policy (@F64116045), and it must not pin EC to
a fixed 3-2 layout (@xichen01).
Some background first, for anyone new to the S3 side
When a client uploads an object it can send a header `x-amz-storage-class:
STANDARD`. In AWS that one word is a bundle of characteristics — how durable it
is, how fast you can read it, how it is priced. AWS deliberately does not let
you pick the pieces separately; you cannot ask for "GLACIER but 3-way
replicated."
In Ozone that bundle currently spans two independent things:
- replication — how many copies, or which EC layout (S3StorageType does this
today)
- storage policy — which disk tier the data sits on: `HOT/WARM/COLD`
(`S3StorageClass`, new in this PR)
Two enums with similar names, as @Gargi-jais11 pointed out, is confusing.
But there is a second problem: if we just add replication as another field on
`S3StorageClass`, then the next characteristic we support — @xichen01 mentioned
zones and latency — means another field again, and potentially new enum entries
for each new combination. That does not scale.
**Proposal: separate the three jobs**
The enum is currently being asked to do three things at once. Splitting them
keeps each one simple:
1. `S3StorageClass` — just the vocabulary. One entry per S3 name, nothing
else:
`REDUCED_REDUNDANCY, STANDARD, STANDARD_IA, GLACIER`. Each entry points at
its spec. This list only changes when AWS-visible names change.
2. `S3StorageClassSpec` — what a class means in Ozone. A small holder with
replicationConfig + storagePolicy. When we later support zones or latency, that
is a new field here — the enum and every caller stay untouched. This is the
extension point.
3. `S3StorageClassResolver` — the mapping rules. Importantly, the two
directions are not symmetric:
- Header → Ozone is strict. An unknown name is a client mistake, so it stays
a 400 InvalidStorageClass, as today.
- Ozone → header must never fail. It is describing data that already exists
on disk, so there is no such thing as an invalid answer. The rule is: storage
policy only promotes the answer; replication is the baseline.
COLD -> GLACIER
WARM + any EC layout -> STANDARD_IA
anything else -> existing replication-based mapping, unchanged
@xichen01 — this directly addresses your EC point. The reverse lookup
matches on replication type (is it EC?), never on the specific 3-2 / 4-2 / 6-3
/ 9-3 layout, so any EC configuration maps back correctly. The layout also
stays operator-selectable through the existing storage-config header, which
S3Utils.toReplicationConfig already honours — that mechanism is unchanged.
@F64116045 — both values are inputs to the reverse lookup, so
REDUCED_REDUNDANCY and STANDARD stay distinguishable even though both are HOT.
And you were right that null is not needed for GLACIER; COLD identifies it on
its own. I am giving GLACIER RATIS THREE so that every storage class is a
complete bundle.
One compatibility issue worth flagging
Something I do not think we have discussed yet, and I would like a second
opinion on.
Mapping from storage policy alone changes what existing objects report. Keys
get a default storage policy of WARM, and WARM → STANDARD_IA. So an ordinary
3-way replicated key that reports STANDARD today would start reporting
STANDARD_IA after the read-path change — in GET, HEAD, ListObjects, ListParts
and GetObjectAttributes. To an S3 client STANDARD_IA implies EC, so that is a
visible behaviour change for data nobody touched.
Making replication the baseline and letting policy only promote avoids this:
a (RATIS THREE, WARM) key still reports STANDARD. Older keys with no stored
policy are also unaffected, because the fallback is the current logic.
Scope of this PR
This PR is infrastructure only — nothing calls `S3StorageClass` yet. I would
like to keep it that way and:
- reshape `S3StorageClass` as above, with unit tests
- mark `S3StorageType` `@Deprecated` and have it delegate to the new resolver
That leaves two type names temporarily but only one implementation, so they
cannot drift apart — which I think is the real risk @Gargi-jais11 raised. All
existing call sites keep working with identical behaviour. The follow-up write
and read PRs migrate the call sites and delete S3StorageType.
--
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]
