Copilot commented on code in PR #11140: URL: https://github.com/apache/ozone/pull/11140#discussion_r3879494238
########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCopyRequest.java: ########## @@ -0,0 +1,326 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.om.request.key; + +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_ALREADY_EXISTS; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.NOT_SUPPORTED_OPERATION; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.LeveledResource.BUCKET_LOCK; + +import java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.apache.hadoop.hdds.utils.db.cache.CacheKey; +import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.audit.OMAction; +import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.OzoneManager; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.om.execution.flowcontrol.ExecutionContext; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; +import org.apache.hadoop.ozone.om.helpers.KeyValueUtil; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; +import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; +import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup; +import org.apache.hadoop.ozone.om.request.OMClientRequestUtils; +import org.apache.hadoop.ozone.om.request.util.OmResponseUtil; +import org.apache.hadoop.ozone.om.response.OMClientResponse; +import org.apache.hadoop.ozone.om.response.key.OMKeyCopyResponse; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CopyKeyRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CopyKeyResponse; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; +import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType; +import org.apache.hadoop.util.Time; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Handles a server side key copy: the destination key is created as an + * independent key that reuses the source key's committed block locations, so no + * data is read or written. Both keys are tagged with a shared block group id + * and the group's sharer count is tracked in the sharedBlockGroupTable, which + * {@link org.apache.hadoop.ozone.om.service.KeyDeletingService} consults so the + * blocks are only released once the last sharer is reclaimed. + * + * <p>This is the proof-of-concept scope. A copy is rejected, and the caller is + * expected to fall back to reading and rewriting the data, when it would need + * to cross a bucket, overwrite an existing key, or touch encrypted, GDPR or + * hsync-active keys. + */ +public class OMKeyCopyRequest extends OMKeyRequest { + + private static final Logger LOG = + LoggerFactory.getLogger(OMKeyCopyRequest.class); + + public OMKeyCopyRequest(OMRequest omRequest, BucketLayout bucketLayout) { + super(omRequest, bucketLayout); + } + + @Override + public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { + CopyKeyRequest copyKeyRequest = + super.preExecute(ozoneManager).getCopyKeyRequest(); + Objects.requireNonNull(copyKeyRequest, "copyKeyRequest == null"); + + KeyArgs sourceKeyArgs = copyKeyRequest.getSourceKeyArgs(); + KeyArgs destinationKeyArgs = copyKeyRequest.getDestinationKeyArgs(); + + if (!sourceKeyArgs.getVolumeName().equals(destinationKeyArgs.getVolumeName()) + || !sourceKeyArgs.getBucketName().equals(destinationKeyArgs.getBucketName())) { + throw new OMException("Server side copy across buckets is not supported yet", + NOT_SUPPORTED_OPERATION); + } + if (sourceKeyArgs.getKeyName().equals(destinationKeyArgs.getKeyName())) { + throw new OMException("Server side copy onto the source key is not supported", + NOT_SUPPORTED_OPERATION); + } + + KeyArgs.Builder normalizedSource = sourceKeyArgs.toBuilder() + .setKeyName(validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(), + sourceKeyArgs.getKeyName(), getBucketLayout())); + KeyArgs.Builder normalizedDestination = destinationKeyArgs.toBuilder() + .setKeyName(validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(), + destinationKeyArgs.getKeyName(), getBucketLayout())) + .setModificationTime(Time.now()); + + KeyArgs resolvedSource = resolveBucketAndCheckKeyAcls(normalizedSource.build(), + ozoneManager, ACLType.READ); + KeyArgs resolvedDestination = resolveBucketAndCheckKeyAcls(normalizedDestination.build(), + ozoneManager, ACLType.CREATE); + + return getOmRequest().toBuilder() + .setUserInfo(getUserInfo()) + .setCopyKeyRequest(copyKeyRequest.toBuilder() + .setSourceKeyArgs(resolvedSource) + .setDestinationKeyArgs(resolvedDestination)) + .build(); + } + + @Override + @SuppressWarnings("checkstyle:methodlength") + public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, ExecutionContext context) { + final long trxnLogIndex = context.getIndex(); Review Comment: This code introduces a new on-disk schema element (`sharedBlockGroupId` in `KeyInfo` + `sharedBlockGroupTable`) and a new request type (`CopyKey`), but the implementation shown here does not gate execution on an `OMLayoutFeature` / finalization check. That gap is dangerous in mixed-version or downgrade scenarios: an older OM will ignore the new proto field and can reclaim shared blocks as if they were exclusively owned, causing silent data loss. To make this safe for anything beyond a draft, add an explicit layout-feature gate at request execution time (and ideally also in key deletion/purge paths) that rejects `CopyKey` and/or disables shared-block behavior until the layout is finalized. ########## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java: ########## @@ -160,13 +171,49 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut AUDIT.logWriteSuccess(ozoneManager.buildAuditMessageForSuccess(OMSystemAction.KEY_DELETION, auditParams)); } return new OMKeyPurgeResponse(omResponse.build(), keysToBePurgedList, renamedKeysToBePurged, fromSnapshotInfo, - keysToUpdateList, bucketInfoList); + keysToUpdateList, bucketInfoList, updatedSharerCounts); } catch (IOException e) { AUDIT.logWriteFailure(ozoneManager.buildAuditMessageForFailure(OMSystemAction.KEY_DELETION, null, e)); return new OMKeyPurgeResponse(createErrorOMResponse(omResponse, e)); } } + /** + * Drops the sharer count of each block group whose sharers were reclaimed in + * this batch, and returns the new counts for the response to persist. A count + * that falls to one is removed instead: the single key left owns the blocks + * again, so its own deletion releases them through the ordinary path. + * + * @return new count per block group id, where a value of one or less means + * the row should be deleted. + */ + private Map<Long, Long> applySharedBlockGroupDecrements( + List<SharedBlockGroupDecrement> decrements, OMMetadataManager omMetadataManager, long trxnLogIndex) + throws IOException { + if (decrements.isEmpty()) { + return Collections.emptyMap(); + } + Map<Long, Long> updatedCounts = new HashMap<>(); + for (SharedBlockGroupDecrement decrement : decrements) { + long groupId = decrement.getSharedBlockGroupId(); + Long currentCount = omMetadataManager.getSharedBlockGroupTable().get(groupId); + if (currentCount == null) { + // Already removed by an earlier batch; nothing left to count down. + continue; + } + long newCount = currentCount - decrement.getSharerCount(); + updatedCounts.put(groupId, newCount); + if (newCount > 1) { + omMetadataManager.getSharedBlockGroupTable().addCacheEntry( + new CacheKey<>(groupId), CacheValue.get(trxnLogIndex, newCount)); + } else { + omMetadataManager.getSharedBlockGroupTable().addCacheEntry( + new CacheKey<>(groupId), CacheValue.get(trxnLogIndex)); + } Review Comment: If `decrement.getSharerCount()` exceeds `currentCount`, `newCount` becomes negative. The subsequent logic treats `newCount <= 1` as a signal to delete the row, which can incorrectly remove the protection for a still-shared group (risking premature block release later). To keep failure modes biased toward leaks rather than data loss, add an explicit guard: if `decrement.getSharerCount() > currentCount` (or `newCount < 0`), log a warning/error and skip applying that decrement (or clamp in a way that preserves the row). ########## hadoop-hdds/docs/content/design/ratis-backed-streaming-readblock.md: ########## @@ -0,0 +1,894 @@ +--- +title: Ratis-Backed Streaming ReadBlock +summary: Design for group-aware and closed-replica ReadBlock over Ratis DataStream +date: 2026-07-11 +status: draft +author: Lixucheng +--- Review Comment: The PR title/description focus on server-side `CopyKey` with shared block groups, but this PR also adds additional (unrelated) design docs (e.g., Ratis-backed `ReadBlock`, deterministic IO fault injection POC). This makes the change set harder to review and reason about. Consider moving unrelated design-doc additions into separate PR(s) so the `CopyKey` implementation and its design doc can be reviewed independently. -- 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]
