utafrali commented on code in PR #11157:
URL: https://github.com/apache/ozone/pull/11157#discussion_r3885680367
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -514,6 +517,64 @@ static void addLastModifiedDate(
RFC1123Util.FORMAT.format(lastModificationTime));
}
+ /**
+ * Adds the {@code x-amz-expiration} header when an enabled lifecycle
+ * expiration rule covers the key, as S3 does: the value names the date the
+ * object is scheduled for deletion and the rule that schedules it. When more
+ * than one rule covers the key, the earliest expiry is reported.
+ * <p>
+ * The header is advisory, so a bucket without a lifecycle configuration, a
+ * caller who may not read it, or any other lookup failure only leaves the
+ * header out; the HEAD itself still succeeds.
+ */
+ private void addExpirationHeader(ResponseBuilder responseBuilder,
+ String bucketName, String keyPath, OzoneKey key) {
+ try {
+ OzoneLifecycleConfiguration lifecycleConfiguration = getClientProtocol()
Review Comment:
`getLifecycleConfiguration` makes a synchronous OM RPC on every `HEAD`
request, including for buckets that have no lifecycle configuration at all. In
that case the call throws an `IOException` on every request, which the catch
block swallows silently. Under any sustained HEAD load on unconfigured buckets
this doubles the OM RPCs and adds latency to every response.
Consider checking whether the bucket carries lifecycle metadata before
fetching the full configuration (the `OzoneBucket` object is already available
when ownership verification is requested, and bucket-level metadata could carry
a flag), or at minimum cache a short-lived negative result per bucket so that
repeated HEAD requests for unconfigured buckets do not each trigger a failed
RPC.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -685,6 +746,7 @@ public Response head(
addLastModifiedDate(response, key);
addTagCountIfAny(response, key);
addCustomMetadataHeaders(response, key);
+ addExpirationHeader(response, bucketName, keyPath, key);
Review Comment:
`addExpirationHeader` is called unconditionally even when `partNumber != 0`.
When a part-number is specified, `key` holds the part's metadata and
`key.getModificationTime()` is the part upload time, not the completed object's
last-modified time. The Days-based expiry calculation in `expiryDateOf` would
therefore use the wrong timestamp and report an incorrect expiry date.
Either skip the expiration header when `partNumber != 0` (S3 does not
document part-level expiration semantics), or fetch the object-level key
separately to obtain the correct modification time before calling
`addExpirationHeader`.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneLifecycleConfiguration.java:
##########
@@ -172,6 +203,23 @@ public OzoneLCAbortIncompleteMultipartUpload
getAbortIncompleteMultipartUpload()
public OzoneLCFilter getFilter() {
return filter;
}
+
+ public boolean isEnabled() {
+ return "Enabled".equals(status);
+ }
+
+ /**
+ * Matches this rule's prefix or filter against a key. Unlike the
+ * server-side OmLCRule#match, which the lifecycle service uses to pick
keys
+ * that are already due for deletion, this only answers whether the rule
+ * covers the key, so callers can report a future expiry date for it.
+ */
+ public boolean matches(String keyPath, Map<String, String> keyTags) {
Review Comment:
`OzoneLCRule.matches` checks `prefix != null` before `filter`, but S3's
lifecycle spec treats the top-level `Prefix` element as deprecated in favour of
`Filter`. A rule can theoretically carry both (during a schema migration). The
comment acknowledges this is a simplified "does the rule cover the key" check,
so the current priority order is acceptable, but it would be worth a brief
inline note explaining why `prefix` wins over `filter` when both are set, so
future maintainers don't inadvertently flip the order.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -514,6 +517,64 @@ static void addLastModifiedDate(
RFC1123Util.FORMAT.format(lastModificationTime));
}
+ /**
+ * Adds the {@code x-amz-expiration} header when an enabled lifecycle
+ * expiration rule covers the key, as S3 does: the value names the date the
+ * object is scheduled for deletion and the rule that schedules it. When more
+ * than one rule covers the key, the earliest expiry is reported.
+ * <p>
+ * The header is advisory, so a bucket without a lifecycle configuration, a
+ * caller who may not read it, or any other lookup failure only leaves the
+ * header out; the HEAD itself still succeeds.
+ */
+ private void addExpirationHeader(ResponseBuilder responseBuilder,
+ String bucketName, String keyPath, OzoneKey key) {
+ try {
+ OzoneLifecycleConfiguration lifecycleConfiguration = getClientProtocol()
+ .getLifecycleConfiguration(key.getVolumeName(), bucketName);
+
+ ZonedDateTime earliest = null;
+ String ruleId = null;
+ for (OzoneLifecycleConfiguration.OzoneLCRule rule :
lifecycleConfiguration.getRules()) {
+ if (!rule.isEnabled() || rule.getExpiration() == null
+ || !rule.matches(keyPath, key.getTags())) {
+ continue;
+ }
+ ZonedDateTime expiryDate = expiryDateOf(rule.getExpiration(),
key.getModificationTime());
+ if (expiryDate != null && (earliest == null ||
expiryDate.isBefore(earliest))) {
+ earliest = expiryDate;
+ ruleId = rule.getId();
+ }
+ }
+
+ if (earliest != null) {
+ responseBuilder.header(EXPIRATION_HEADER,
+ String.format("expiry-date=\"%s\", rule-id=\"%s\"",
RFC1123Util.FORMAT.format(earliest), ruleId));
Review Comment:
`ruleId` is taken directly from `rule.getId()` without a null guard. If a
rule ever lacks an ID (not enforced at this layer), the header becomes
`rule-id="null"`, which is malformed. A simple `continue` when `rule.getId() ==
null` would avoid the bad value and is consistent with how the loop already
skips rules without expiration.
##########
hadoop-ozone/dist/src/main/smoketest/s3/bucketlifecycle.robot:
##########
@@ -61,3 +61,24 @@ Delete bucket lifecycle configuration when none exists
${bucket} = Create bucket
${result} = Execute AWSS3APICli delete-bucket-lifecycle
--bucket ${bucket}
Should Be Empty ${result}
+
+Head object reports expiration of matching lifecycle rule
Review Comment:
The two new robot cases pin only that the `Expiration` header is present or
absent, but do not assert its content (the expiry date and rule-id). Given that
the PR author explicitly calls out the exact header format in the unit tests,
adding a `Should Match Regexp` assertion on the value would make the smoke test
catch format regressions in a compose environment where the unit tests do not
run.
--
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]
