devmadhuu commented on code in PR #11377:
URL: https://github.com/apache/ozone/pull/11377#discussion_r4228959722
##########
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:
Great catch !. Thanks for pointing it out. Now setting storagePolicy in
keyArgs for `BlockOutputStreamEntryPool` also. Applied both changes as
suggested, and added `testStoragePolicyHonouredOnSubsequentBlockAllocation`. It
creates a COLD key in a WARM bucket with size 0, so every block comes from
allocateBlock, writes 2+ blocks, and checks that every block is on ARCHIVE, not
just the first.
##########
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:
Applied, and added a test for a policy-changing overwrite. One related case
I'd like your view on: an overwrite that sends no policy still keeps the old
one, since the new `if` condition skips it — but allocation falls back to the
bucket. So a HOT key rewritten without a flag in a WARM bucket lands on DISK
while the record still says HOT. Should a no-policy overwrite clear the field,
like a fresh create does, or should allocation honor the key's existing policy?
##########
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:
You're right — nothing populates it. Multipart storage policy support is the
scope of the follow-up patch, which records the policy at initiate in
`S3InitiateMultipartUploadRequest`, uses it for part allocation, and copies it
at completion. I'll remove it here so this PR covers single-part keys only, and
cover the new-object and overwrite multipart cases in the multipart PR.
##########
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:
The datastream path (`BlockDataStreamOutputEntryPool` →
`BlockDataStreamOutput`) is separate from the one this PR changes, and this PR
doesn't touch it, so you're right that a streamed write won't carry the tier.
Streaming storage-policy support is a follow-up patch: it carries the storage
type through `BlockDataStreamOutputEntry` and
`BlockDataStreamOutput.setupStream()` into the block ID, and follow up patch
will also add a streaming-write test that checks the physical volume. I'd like
to keep it there so this PR stays focused on the standard write path. I'll make
sure the streaming test checks the volume, not just the recorded policy, as you
suggested.
--
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]
