smengcl commented on code in PR #11377:
URL: https://github.com/apache/ozone/pull/11377#discussion_r4216156505
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java:
##########
@@ -108,8 +108,11 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
// To allocate atleast one block passing requested size and scmBlockSize
// as same value. When allocating block requested size is same as
// scmBlockSize.
+ final OmBucketInfo bucketInfo = ozoneManager
+ .getBucketInfo(keyArgs.getVolumeName(), keyArgs.getBucketName());
final List<OmKeyLocationInfo> omKeyLocationInfoList =
allocateBlock(repConfig, excludeList,
- ozoneManager.getScmBlockSize(), keyArgs.getSortDatanodes(), userInfo,
ozoneManager);
+ ozoneManager.getScmBlockSize(), keyArgs.getSortDatanodes(), userInfo,
ozoneManager,
+ getStoragePolicy(bucketInfo, keyArgs),
getAllowFallbackStoragePolicy(bucketInfo));
Review Comment:
Subsequent allocations lose the key-level policy:
`BlockOutputStreamEntryPool` does not copy it from the open key, and the
allocate-block translator does not serialize it. For example, a COLD key in a
WARM bucket switches to DISK after its preallocated blocks run out. Creating it
with size zero hits this on the first write.
Can we forward the resolved policy through both places and add a test that
requires another allocation?
```diff
---
a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java
+++
b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java
@@ -103,6 +103,7 @@
.setBucketName(info.getBucketName()).setKeyName(info.getKeyName())
.setReplicationConfig(b.getReplicationConfig())
.setDataSize(info.getDataSize())
+ .setStoragePolicy(info.getStoragePolicy())
.setIsMultipartKey(b.isMultipartKey())
.setMultipartUploadID(b.getMultipartUploadID())
.setMultipartUploadPartNumber(b.getMultipartNumber());
---
a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
+++
b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
@@ -815,6 +815,10 @@
keyArgs.setType(args.getReplicationConfig().getReplicationType());
}
+ if (args.getStoragePolicy() != null) {
+
keyArgs.setStoragePolicy(OzoneStoragePolicy.toProto(args.getStoragePolicy()));
+ }
+
req.setKeyArgs(keyArgs);
req.setClientID(clientId);
req.setExcludeList(excludeList.getProtoBuf());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -1094,6 +1124,9 @@ protected OmKeyInfo createFileInfo(
if (keyArgs.hasExpectedDataGeneration()) {
builder.setExpectedDataGeneration(keyArgs.getExpectedDataGeneration());
}
+ if (keyArgs.hasStoragePolicy()) {
Review Comment:
This records the policy for new keys, but the existing-key branch in
`prepareFileInfo()` copies the old policy unchanged. Overwriting a HOT key with
COLD allocates ARCHIVE blocks while `getStoragePolicy()` still reports HOT.
Can we update the overwrite builder too and cover a policy-changing
overwrite?
```diff
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java
@@ -1074,6 +1074,9 @@
if (keyArgs.hasExpectedDataGeneration()) {
builder.setExpectedDataGeneration(keyArgs.getExpectedDataGeneration());
}
+ if (keyArgs.hasStoragePolicy()) {
+
builder.setStoragePolicy(OzoneStoragePolicy.fromProto(keyArgs.getStoragePolicy()));
+ }
return builder.build();
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java:
##########
@@ -543,6 +543,9 @@ protected OmKeyInfo getOmKeyInfo(long trxnLogIndex,
if (dbOpenKeyInfo.getTags() != null) {
builder.setTags(dbOpenKeyInfo.getTags());
}
+ if (dbOpenKeyInfo.getStoragePolicy() != null) {
Review Comment:
Where is this policy populated? `S3InitiateMultipartUploadRequest` never
sets it on the multipart open key, and the multipart branch of
`OMKeyCreateRequest.preExecute()` also skips policy resolution. This condition
therefore stays false: new multipart objects report null, and multipart
overwrites retain the previous object's policy.
Can we resolve and record the policy during initiation, then reuse it for
part allocation and completion? Please cover both a new multipart object and an
overwrite.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java:
##########
@@ -168,9 +170,23 @@ BlockOutputStreamEntry createStreamEntry(OmKeyLocationInfo
subKeyInfo, boolean f
.setStreamBufferArgs(streamBufferArgs)
.setExecutorServiceSupplier(executorServiceSupplier)
.setForRetry(forRetry)
+ .setStorageType(getStorageType(subKeyInfo))
Review Comment:
Can we propagate the selected storage type through the Ratis datastream path
too? `BlockDataStreamOutputEntryPool` drops the tier, and
`BlockDataStreamOutput.setupStream()` sends a block ID without a storage type.
Consequently, `createStreamKey()` in a COLD bucket can create the datanode
container on DISK even though SCM selected ARCHIVE. This also affects S3 writes
when datastream is enabled.
Please carry the type through stream initialization and block metadata, and
verify the physical volume in a streaming-write test.
--
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]
