amit-jain commented on PR #2989:
URL: https://github.com/apache/jackrabbit-oak/pull/2989#issuecomment-4968910450
Reviewed the v12 migration focusing on correctness, resource lifecycle, and
listing/iteration memory behavior. The core rewrite looks careful —
`getAllIdentifiers`/`getAllRecords` stream lazily via `PagedIterable` (no
full-listing materialization), client/credential/delegation-key caching avoids
per-call Netty allocation and OAuth round-trips, the `sig=` redaction in the
logging policy is correct, and the fixed 64 MiB upload block size is a sensible
OOM fix over v8's `min(len, 4GB)`. Two things worth addressing before merge:
## 1. Secondary-location failover is silently disabled unless
`maxErrorRetry` is also set (behavior regression vs v8)
In `AzureBlobStoreBackendV12.initAzureDSConfig()`,
`computeSecondaryLocationEndpoint()` correctly resolves the secondary endpoint
when `enableSecondaryLocation=true`, but it is only passed through as an
argument to `UtilsV12.getRetryOptions(...)`:
```java
retryOptions = UtilsV12.getRetryOptions(
properties.getProperty(AzureConstantsV12.AZURE_BLOB_MAX_REQUEST_RETRY),
requestTimeout,
computeSecondaryLocationEndpoint());
```
`getRetryOptions()` returns `null` *before* it ever inspects
`secondaryLocation`, whenever `maxErrorRetry` is unset:
```java
public static RequestRetryOptions getRetryOptions(final String
maxRequestRetryCount, Integer requestTimeout, String secondaryLocation) {
int retries = PropertiesUtil.toInteger(maxRequestRetryCount, -1);
if (retries < 0) {
return null; // secondaryLocation discarded here
}
...
}
```
and `AzureBlobContainerProviderV12` only wires the secondary host into the
client when `retryOptions != null`:
```java
if (retryOptions != null) {
builder.retryOptions(retryOptions);
}
```
**Net effect:** an operator who sets `enableSecondaryLocation=true` without
also setting `maxErrorRetry` gets no secondary-region failover at all,
silently. On the v8 backend (`AzureBlobStoreBackend.getBlobRequestOptions()`),
`enableSecondaryLocation` and the retry policy were independent `if` blocks on
the same `BlobRequestOptions`, so secondary-location worked regardless of retry
config — this is a regression for anyone relying on that independence, and
there's no test covering the combination.
**Suggested fix** — build retry options whenever *either* retries or a
secondary location is configured, so a secondary-location-only config still
gets a `RequestRetryOptions` (with SDK-default retry count) carrying the
`secondaryHost`:
```java
public static RequestRetryOptions getRetryOptions(final String
maxRequestRetryCount, Integer requestTimeout, String secondaryLocation) {
int retries = PropertiesUtil.toInteger(maxRequestRetryCount, -1);
if (retries < 0 && secondaryLocation == null) {
return null;
}
RetryPolicyType type = retries > 0 ? RetryPolicyType.EXPONENTIAL :
RetryPolicyType.FIXED;
int maxTries = retries >= 0 ? Math.max(retries, 1) : /* SDK default,
e.g. 4 */ 4;
return new RequestRetryOptions(type, maxTries, requestTimeout, null,
null, secondaryLocation);
}
```
(Exact default retry count is the author's call — the key point is that the
`retries < 0` short-circuit must also account for `secondaryLocation != null`.)
Worth adding a test asserting `getRetryOptions(null, timeout,
"https://foo-secondary...";)` returns non-null with the secondary host set.
## 2. (Minor / hardening) Presigned-URI expiry above 7 days silently breaks
under service-principal auth
`getOrRefreshDelegationKey()` always requests the user delegation key for a
fixed 7-day `DELEGATION_KEY_LIFETIME`. If
`presignedHttpDownloadURIExpirySeconds` / `...UploadURIExpirySeconds` is
configured above 7 days, then `sasExpiry > keyExpiry`, which has two silent
consequences:
1. The fast-path cache check
`cached.expiry.isAfter(sasExpiry.plus(RENEWAL_BUFFER))` can never be satisfied,
so every presigned-URI call makes a fresh `getUserDelegationKey` round-trip —
defeating the cache the class was built around (the O(N)-per-part path it
explicitly avoids).
2. The SAS is signed by a key that expires before the SAS's own stated
expiry, so the URI stops working after 7 days regardless of the configured
expiry.
Presigned URIs are disabled by default (expiry `0`), so this only affects
operators who deliberately configure a >7-day expiry — but it fails with no
warning. Consider validating/capping the configured expiry at
`DELEGATION_KEY_LIFETIME` (with a log warning) so the misconfiguration is
visible.
--
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]
