Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
74 commits
Select commit Hold shift + click to select a range
853bb3b
Create outline for open key delete request
errose28 Sep 1, 2020
b1b0ac0
Add DeleteOpenKeyRequest proto, and store updated OMKeyInfo for the r…
errose28 Sep 1, 2020
ace8408
Add empty class outline for open key delete response
errose28 Sep 1, 2020
c2043a2
Merge branch 'master' into HDDS-4122
errose28 Sep 1, 2020
73734ea
Create first draft of AbstractOMKeyDeleteResponse
errose28 Sep 3, 2020
5e9f881
Move common code from key delete requests into abstract class
errose28 Sep 10, 2020
401723d
Merge branch 'HDDS-4122-abstract-om-response' into HDDS-4122
errose28 Sep 10, 2020
e3f4ec2
Move audit log and metrics to helper methods
errose28 Sep 10, 2020
1576342
Minor comment and format changes to open key delete request
errose28 Sep 10, 2020
c2057fb
Add helper methods needed to write unit tests
errose28 Sep 11, 2020
4fd8535
Refactor testing utils to easier manipulate keys
errose28 Sep 11, 2020
cb87f6d
First implementation of unit tests for OMOpenKeyDeleteRequest
errose28 Sep 14, 2020
57d62f3
Comment out unfinished audit log and metrics code for testing
errose28 Sep 14, 2020
53f75a3
Add protos to better represent open keys to delete
errose28 Sep 14, 2020
9731b72
Update proto layout and incorporate changes to OMOpenKeyDeleteRequest
errose28 Sep 14, 2020
cea208f
All unit tests pass with new proto structure
errose28 Sep 14, 2020
8b2fe23
Merge branch 'HDDS-4122-unit-tests-new-openkey-proto' into HDDS-4122-…
errose28 Sep 14, 2020
41f1013
Implement unit tests for OMOpenKeyDeleteResponse
errose28 Sep 15, 2020
6222cd3
Pass key names to OMResponse for open keys, since their names cannot …
errose28 Sep 15, 2020
efbc89c
Add documnetation to OMOpenKeyDeleteResponse unit tests
errose28 Sep 15, 2020
c356fe9
Merge branch 'HDDS-4122-response-unit-tests' into HDDS-4122-unit-tests
errose28 Sep 15, 2020
be2bacc
Merge branch 'HDDS-4122-unit-tests' into HDDS-4122
errose28 Sep 15, 2020
2c00443
Rename Id to clientID in OpenKey proto
errose28 Sep 15, 2020
46ea78d
Add logging to OMOpenKeyDeleteRequest
errose28 Sep 16, 2020
c800c8b
Implement metrics for OMOpenKeyDeleteRequest
errose28 Sep 17, 2020
e37d066
Add unit test for OMOpenKeyDeleteRequest metrics, and fix metrics bugs
errose28 Sep 17, 2020
b61b529
Merge branch 'HDDS-4122-log-and-metrics' into HDDS-4122
errose28 Sep 17, 2020
181d705
Replace table get calls with isExist where applicable
errose28 Sep 17, 2020
82f3386
Give variables more intuitive names and update test documentation
errose28 Sep 17, 2020
323c8cd
Fix checkstyle errors
errose28 Sep 17, 2020
90ab92d
Merge branch 'master' into HDDS-4122-quota-attempt2
errose28 Sep 18, 2020
574d280
Add volume quota update to open key delete response, and group duplic…
errose28 Sep 18, 2020
fe52679
Move common volume byte usage update code to AbstractOMKeyDeleteResponse
errose28 Sep 18, 2020
998cd09
Remove unused inports and fix super constructor calls
errose28 Sep 18, 2020
e7c5ae1
Remove redundant check status calls in children of AbstractOMOpenKeyD…
errose28 Sep 18, 2020
5fccc60
Incorporate volume size tracking into open key commit request and res…
errose28 Sep 18, 2020
918a820
Update volume table cache in OMOpenKeyDeleteRequest
errose28 Sep 18, 2020
1ab1a80
Support specifying a log transaction index in AbstractOMKeyDeleteResp…
errose28 Sep 18, 2020
5b06196
Merge branch 'HDDS-4122' into HDDS-4122-volume-quota
errose28 Sep 21, 2020
6f52ea5
Make unit tests build by using in dummy volume args object
errose28 Sep 21, 2020
5b9fd76
Only update volume quota if volume still exists.
errose28 Sep 21, 2020
528a5f4
Merge branch 'HDDS-4122' into HDDS-4122-volume-usage
errose28 Sep 21, 2020
ebf0bed
Add todo comments with testing plans
errose28 Sep 21, 2020
3700c55
Add DeleteOpenKeys as a non-readonly operation
errose28 Sep 21, 2020
6d42791
Add table cleanup annotation to AbstractOMKeyDeleteResponse
errose28 Sep 21, 2020
ab18e1b
Fix unit test for OMKeysDeleteResponse that expected empty keys to be…
errose28 Sep 21, 2020
8220b2f
Add check for volume existance before updating volume byte usage
errose28 Sep 21, 2020
dba1763
Remove unused imports
errose28 Sep 21, 2020
be34b22
Merge branch 'HDDS-4122' into HDDS-4122-volume-usage
errose28 Sep 22, 2020
cae3260
Fix cache update for volume info in OMOpenKeyDeleteRequest
errose28 Sep 22, 2020
53778c3
Merge branch 'HDDS-4122' into HDDS-4122-volume-usage
errose28 Sep 22, 2020
bec8cba
Rename method for updating volume args, and remove check in response …
errose28 Sep 23, 2020
3d3cef2
Add volume update test to unit tests for OMOpenKeyDeleteResponse
errose28 Sep 23, 2020
51b3dd2
Remove uneeded exception in method signature
errose28 Sep 23, 2020
bde9a85
Move creation of blocks for keyinfo to TestOMRequestUtils
errose28 Sep 28, 2020
2449f09
Add passing test for volume byte usage update
errose28 Sep 28, 2020
d528da3
Update documentation and method names in TestOMOpenKeyDeleteRequest
errose28 Sep 28, 2020
acdeef6
Update docs and method names in unit tests
errose28 Sep 28, 2020
65ca25e
Merge branch 'HDDS-4122-volume-usage' into HDDS-4122
errose28 Sep 28, 2020
29969f2
Fix checkstyle errors
errose28 Sep 28, 2020
d46ab13
Make plural of keys consistent in file and class names
errose28 Sep 28, 2020
9d137a1
Revert other delete request/response files back to their original sta…
errose28 Sep 28, 2020
82e70a6
Merge branch 'master' into HDDS-4122-remove-code-consolidation
errose28 Sep 28, 2020
fbcb784
Restore files that had deduplicated code from master
errose28 Sep 28, 2020
3cb1e2c
Merge branch 'HDDS-4122-remove-code-consolidation' into HDDS-4122
errose28 Sep 28, 2020
14a2241
Rename method getBytesUsed -> sumBlockLengths
errose28 Sep 28, 2020
c6ede36
Fix minor issues from PR
errose28 Oct 6, 2020
ff03957
Remove volume byte usage update code and test cases
errose28 Oct 6, 2020
8d3ca6f
Move misplaced @Test annotation
errose28 Oct 6, 2020
4619c29
Consolidate duplicate code among key delete requests and responses
errose28 Oct 6, 2020
b1c200d
Fix checkstyle violations and accidental code deletion
errose28 Oct 6, 2020
089605d
Merge remote-tracking branch 'origin/HDDS-4122' into HDDS-4122
errose28 Oct 6, 2020
b292bc2
Make OMKeysDeleteRequest set the updateID of each OmKeyInfo to the lo…
errose28 Oct 9, 2020
2f84e95
Remove unused trxnLogIndex field from OMKeysDeleteResponse
errose28 Oct 9, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ public static boolean isReadOnly(
case AddAcl:
case PurgeKeys:
case RecoverTrash:
case DeleteOpenKeys:
return false;
default:
LOG.error("CmdType {} is not categorized as readOnly or not.", cmdType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ enum Type {
AllocateBlock = 37;
DeleteKeys = 38;
RenameKeys = 39;
DeleteOpenKeys = 40;

InitiateMultiPartUpload = 45;
CommitMultiPartUpload = 46;
Expand Down Expand Up @@ -128,6 +129,7 @@ message OMRequest {
optional AllocateBlockRequest allocateBlockRequest = 37;
optional DeleteKeysRequest deleteKeysRequest = 38;
optional RenameKeysRequest renameKeysRequest = 39;
optional DeleteOpenKeysRequest deleteOpenKeysRequest = 40;

optional MultipartInfoInitiateRequest initiateMultiPartUploadRequest = 45;
optional MultipartCommitUploadPartRequest commitMultiPartUploadRequest = 46;
Expand Down Expand Up @@ -929,6 +931,21 @@ message PurgeKeysResponse {

}

message DeleteOpenKeysRequest {
repeated OpenKeyBucket openKeysPerBucket = 1;
}

message OpenKeyBucket {
required string volumeName = 1;
required string bucketName = 2;
repeated OpenKey keys = 3;
}

message OpenKey {
required string name = 1;
required uint64 clientID = 2;
}

message OMTokenProto {
enum Type {
DELEGATION_TOKEN = 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ public class OMMetrics {
private @Metric MutableCounterLong numLookupFile;
private @Metric MutableCounterLong numListStatus;

private @Metric MutableCounterLong numOpenKeyDeleteRequests;
private @Metric MutableCounterLong numOpenKeysSubmittedForDeletion;
private @Metric MutableCounterLong numOpenKeysDeleted;

// Failure Metrics
private @Metric MutableCounterLong numVolumeCreateFails;
private @Metric MutableCounterLong numVolumeUpdateFails;
Expand Down Expand Up @@ -103,6 +107,7 @@ public class OMMetrics {
private @Metric MutableCounterLong numAbortMultipartUploadFails;
private @Metric MutableCounterLong numListMultipartUploadParts;
private @Metric MutableCounterLong numListMultipartUploadPartFails;
private @Metric MutableCounterLong numOpenKeyDeleteRequestFails;

private @Metric MutableCounterLong numGetFileStatusFails;
private @Metric MutableCounterLong numCreateDirectoryFails;
Expand Down Expand Up @@ -539,6 +544,22 @@ public void incNumCheckpointFails() {
numCheckpointFails.incr();
}

public void incNumOpenKeyDeleteRequests() {
numOpenKeyDeleteRequests.incr();
}

public void incNumOpenKeysSubmittedForDeletion(long amount) {
numOpenKeysSubmittedForDeletion.incr(amount);
}

public void incNumOpenKeysDeleted() {
numOpenKeysDeleted.incr();
}

public void incNumOpenKeyDeleteRequestFails() {
numOpenKeyDeleteRequestFails.incr();
}

@VisibleForTesting
public long getNumVolumeCreates() {
return numVolumeCreates.value();
Expand Down Expand Up @@ -795,6 +816,22 @@ public long getLastCheckpointStreamingTimeTaken() {
return lastCheckpointStreamingTimeTaken.value();
}

public long getNumOpenKeyDeleteRequests() {
return numOpenKeyDeleteRequests.value();
}

public long getNumOpenKeysSubmittedForDeletion() {
return numOpenKeysSubmittedForDeletion.value();
}

public long getNumOpenKeysDeleted() {
return numOpenKeysDeleted.value();
}

public long getNumOpenKeyDeleteRequestFails() {
return numOpenKeyDeleteRequestFails.value();
}

public void unRegister() {
MetricsSystem ms = DefaultMetricsSystem.instance();
ms.unregisterSource(SOURCE_NAME);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@

import com.google.common.base.Optional;
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
Expand Down Expand Up @@ -145,15 +143,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
keyName)),
new CacheValue<>(Optional.absent(), trxnLogIndex));

long quotaReleased = 0;
int keyFactor = omKeyInfo.getFactor().getNumber();
omVolumeArgs = getVolumeInfo(omMetadataManager, volumeName);
omBucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName);
OmKeyLocationInfoGroup keyLocationGroup =
omKeyInfo.getLatestVersionLocations();
for(OmKeyLocationInfo locationInfo: keyLocationGroup.getLocationList()){
quotaReleased += locationInfo.getLength() * keyFactor;
}

long quotaReleased = sumBlockLengths(omKeyInfo);
// update usedBytes atomically.
omVolumeArgs.getUsedBytes().add(-quotaReleased);
omBucketInfo.getUsedBytes().add(-quotaReleased);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.om.PrefixManager;
Expand Down Expand Up @@ -575,17 +576,43 @@ protected boolean checkDirectoryAlreadyExists(String volumeName,
}

/**
* Return volume info for the specified volume.
* Return volume info for the specified volume. If the volume does not
* exist, returns {@code null}.
* @param omMetadataManager
* @param volume
* @return OmVolumeArgs
* @throws IOException
*/
protected OmVolumeArgs getVolumeInfo(OMMetadataManager omMetadataManager,
String volume) {
return omMetadataManager.getVolumeTable().getCacheValue(
new CacheKey<>(omMetadataManager.getVolumeKey(volume)))
.getCacheValue();

OmVolumeArgs volumeArgs = null;

CacheValue<OmVolumeArgs> value =
omMetadataManager.getVolumeTable().getCacheValue(
new CacheKey<>(omMetadataManager.getVolumeKey(volume)));

if (value != null) {
volumeArgs = value.getCacheValue();
}

return volumeArgs;
}

/**
* @return the number of bytes used by blocks pointed to by {@code omKeyInfo}.
*/
protected static long sumBlockLengths(OmKeyInfo omKeyInfo) {
long bytesUsed = 0;
int keyFactor = omKeyInfo.getFactor().getNumber();
OmKeyLocationInfoGroup keyLocationGroup =
omKeyInfo.getLatestVersionLocations();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point to update the used bytes while doing this cleanup. I am wondering what this would mean with multiple key version support in the future. We do not seem to store the "version" of the current open key.


for(OmKeyLocationInfo locationInfo: keyLocationGroup.getLocationList()) {
bytesUsed += locationInfo.getLength() * keyFactor;
}

return bytesUsed;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
import org.apache.hadoop.ozone.om.ResolvedBucket;
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
Expand Down Expand Up @@ -167,12 +165,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
omKeyInfo.getKeyName())),
new CacheValue<>(Optional.absent(), trxnLogIndex));

int keyFactor = omKeyInfo.getFactor().getNumber();
OmKeyLocationInfoGroup keyLocationGroup =
omKeyInfo.getLatestVersionLocations();
for(OmKeyLocationInfo locationInfo: keyLocationGroup.getLocationList()){
quotaReleased += locationInfo.getLength() * keyFactor;
}
omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
quotaReleased += sumBlockLengths(omKeyInfo);
}
// update usedBytes atomically.
omVolumeArgs.getUsedBytes().add(-quotaReleased);
Expand All @@ -182,7 +176,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
.setDeleteKeysResponse(DeleteKeysResponse.newBuilder()
.setStatus(deleteStatus).setUnDeletedKeys(unDeletedKeys))
.setStatus(deleteStatus ? OK : PARTIAL_DELETE)
.setSuccess(deleteStatus).build(), omKeyInfoList, trxnLogIndex,
.setSuccess(deleteStatus).build(), omKeyInfoList,
ozoneManager.isRatisEnabled(), omVolumeArgs, omBucketInfo);

result = Result.SUCCESS;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* 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
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* 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.response.key;

import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.om.OMMetadataManager;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo;
import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
import org.apache.hadoop.ozone.om.response.OMClientResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos
.OMResponse;
import org.apache.hadoop.hdds.utils.db.BatchOperation;

import java.io.IOException;
import javax.annotation.Nullable;
import javax.annotation.Nonnull;

import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.DELETED_TABLE;

/**
* Base class for responses that need to move keys from an arbitrary table to
* the deleted table.
*/
@CleanupTableInfo(cleanupTables = {DELETED_TABLE})
public abstract class AbstractOMKeyDeleteResponse extends OMClientResponse {

private boolean isRatisEnabled;

public AbstractOMKeyDeleteResponse(
@Nonnull OMResponse omResponse, boolean isRatisEnabled) {

super(omResponse);
this.isRatisEnabled = isRatisEnabled;
}

/**
* For when the request is not successful.
* For a successful request, the other constructor should be used.
*/
public AbstractOMKeyDeleteResponse(@Nonnull OMResponse omResponse) {
super(omResponse);
checkStatusNotOK();
}

/**
* Adds the operation of deleting the {@code keyName omKeyInfo} pair from
* {@code fromTable} to the batch operation {@code batchOperation}. The
* batch operation is not committed, so no changes are persisted to disk.
* The log transaction index used will be retrieved by calling
* {@link OmKeyInfo#getUpdateID} on {@code omKeyInfo}.
*/
protected void addDeletionToBatch(
OMMetadataManager omMetadataManager,
BatchOperation batchOperation,
Table<String, ?> fromTable,
String keyName,
OmKeyInfo omKeyInfo) throws IOException {

// For OmResponse with failure, this should do nothing. This method is
// not called in failure scenario in OM code.
fromTable.deleteWithBatch(batchOperation, keyName);

// If Key is not empty add this to delete table.
if (!isKeyEmpty(omKeyInfo)) {
// If a deleted key is put in the table where a key with the same
// name already exists, then the old deleted key information would be
// lost. To avoid this, first check if a key with same name exists.
// deletedTable in OM Metadata stores <KeyName, RepeatedOMKeyInfo>.
// The RepeatedOmKeyInfo is the structure that allows us to store a
// list of OmKeyInfo that can be tied to same key name. For a keyName
// if RepeatedOMKeyInfo structure is null, we create a new instance,
// if it is not null, then we simply add to the list and store this
// instance in deletedTable.
RepeatedOmKeyInfo repeatedOmKeyInfo =
omMetadataManager.getDeletedTable().get(keyName);
repeatedOmKeyInfo = OmUtils.prepareKeyForDelete(
omKeyInfo, repeatedOmKeyInfo, omKeyInfo.getUpdateID(),
isRatisEnabled);
omMetadataManager.getDeletedTable().putWithBatch(
batchOperation, keyName, repeatedOmKeyInfo);
}
}

@Override
public abstract void addToDBBatch(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException;

/**
* Check if the key is empty or not. Key will be empty if it does not have
* blocks.
*
* @param keyInfo
* @return if empty true, else false.
*/
private boolean isKeyEmpty(@Nullable OmKeyInfo keyInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks some of the logic is common for OMKeyDeleteResponse and OMOpenKeysDeleteResponse like isKeyEmpty and deleteFromTable can be used from OMKeyDeleteResponse.
Can we consolidate them and use this AbstractOMKeyDeleteResponse as base class for both of them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the idea of creating this abstract class was to eventually consolidate the duplicate code between OMOpenKeysDeleteResponse, OMKeyDeleteResponse, and OMKeysDeleteResponse. I had originally refactored the other classes as well to use this code, but since HDDS-451 (quota support) is moving along at a brisk pace, I could not keep up with the merge conflicts as the other response classes kept changing, and decided it was better to do this in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like development on the key(s) delete request and response classes has taken a break. I have refactored them to use these shared methods now.

if (keyInfo == null) {
return true;
}
for (OmKeyLocationInfoGroup keyLocationList : keyInfo
.getKeyLocationVersions()) {
if (keyLocationList.getLocationListCount() != 0) {
return false;
}
}
return true;
}
}
Loading