amit-jain commented on code in PR #2621:
URL: https://github.com/apache/jackrabbit-oak/pull/2621#discussion_r2554233391
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/Utils.java:
##########
@@ -372,16 +408,24 @@ static String calculateMD5(final String customerKey) {
}
private static ClientOverrideConfiguration
getClientConfiguration(Properties prop) {
+ final boolean isS3 = Objects.equals(RemoteStorageMode.S3,
prop.get(S3Constants.MODE));
+
int maxErrorRetry =
Integer.parseInt(prop.getProperty(S3Constants.S3_MAX_ERR_RETRY));
int connectionTimeOut =
Integer.parseInt(prop.getProperty(S3Constants.S3_CONN_TIMEOUT));
String encryptionType = prop.getProperty(S3Constants.S3_ENCRYPTION);
+ // API timeout should be much longer than connection timeout for large
file uploads
+ // Use at least 5 minutes, or 10x connection timeout, whichever is
larger
+ int apiTimeout = Math.max(connectionTimeOut * 10, 300000); // At least
5 minutes
Review Comment:
Some heuristic applied, is there any best practice/recommendation on this?
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -528,15 +530,6 @@ public void addMetadataRecord(final InputStream input,
final String name) throws
if (contextClassLoader != null) {
Thread.currentThread().setContextClassLoader(contextClassLoader);
}
- executor.shutdown();
Review Comment:
Why has the closing of the executor been removed? This looks an unintended
change
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/Utils.java:
##########
@@ -482,6 +574,17 @@ private static void configureBuilder(final
S3BaseClientBuilder builder, final Pr
builder.endpointOverride(getEndPointUri(prop, accReq, region));
builder.crossRegionAccessEnabled(Boolean.parseBoolean(prop.getProperty(S3Constants.S3_CROSS_REGION_ACCESS)));
+
+ // Disable checksums (replaces deprecated checksumValidationEnabled)
+
builder.requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED);
+
builder.responseChecksumValidation(ResponseChecksumValidation.WHEN_REQUIRED);
+
Review Comment:
Again defaults changed?
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/Utils.java:
##########
@@ -410,12 +473,39 @@ private static SdkHttpClient getSdkHttpClient(Properties
prop) {
private static SdkAsyncHttpClient getSdkAsyncHttpClient(Properties prop) {
HttpClientConfig config = new HttpClientConfig(prop);
+ final boolean isGCP = Objects.equals(RemoteStorageMode.GCP,
prop.get(S3Constants.MODE));
final NettyNioAsyncHttpClient.Builder builder =
NettyNioAsyncHttpClient.builder();
- builder.connectionTimeout(Duration.ofMillis(config.connectionTimeout))
+ // Calculate connection lifecycle based on socket timeout (all in
SECONDS)
+ long socketTimeoutSeconds = config.socketTimeout / 1000;
+
+ // Idle time: 2x socket timeout (min 30s, max 120s)
+ long idleTimeSeconds = Math.min(120, Math.max(30, socketTimeoutSeconds
* 2));
+
+ // TTL: 5x socket timeout (min 60s, max 600s = 10min)
+ long ttlSeconds = Math.min(600, Math.max(60, socketTimeoutSeconds *
5));
+
+ // GCP needs higher concurrency (no HTTP/2, so more connections needed)
+ final int concurrency = isGCP ? Math.max(100, config.maxConnections) :
Math.max(50, config.maxConnections);
+
+ // More threads for GCP
+ final int threads = isGCP ? Math.max(16,
Runtime.getRuntime().availableProcessors() * 2)
+ : Math.max(4, Runtime.getRuntime().availableProcessors());
+
+
+ builder.connectionTimeout(Duration.ofMillis(config.connectionTimeout))
// Connection timeouts
.readTimeout(Duration.ofMillis(config.socketTimeout))
.writeTimeout(Duration.ofMillis(config.socketTimeout))
- .maxConcurrency(config.maxConnections);
+ .maxConcurrency(concurrency) // Connection pool - increased
for better concurrency
+ .connectionMaxIdleTime(Duration.ofSeconds(idleTimeSeconds))
+ .connectionTimeToLive(Duration.ofSeconds(ttlSeconds))
+ .useIdleConnectionReaper(true)
+ .connectionAcquisitionTimeout(Duration.ofSeconds(10)) // Don't
wait too long for a connection from pool
Review Comment:
No config, changing the defaults os Ok?
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -514,11 +514,13 @@ public void addMetadataRecord(final InputStream input,
final String name) throws
try {
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
// Specify `null` for the content length when you don't know the
content length.
- final AsyncRequestBody body =
AsyncRequestBody.fromInputStream(input, null, executor);
+ byte[] bytes = input.readAllBytes();
Review Comment:
Reading all input to memory will cause poetntial OOMs, these files can be
quite big
--
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]
