Skip to content

HDDS-7526. Avoid overwriting replication config on existing bucket when quota is set #4013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sodonnel merged 6 commits into from
Dec 6, 2022
Merged

HDDS-7526. Avoid overwriting replication config on existing bucket when quota is set #4013

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1bada1
HDDS-7526: Avoid overwriting replication config on existing bucket wh…
Nov 29, 2022
651ca6c
Merge branch 'apache:master' into HDDS-7526
SaketaChalamchala Nov 29, 2022
0cbf5ad
HDDS-7526. Avoid overwriting replication config on existing bucket wh…
Nov 30, 2022
91d8fde
HDDS-7526. Avoid overwriting replication config on existing bucket wh…
Nov 30, 2022
1495d3e
HDDS-7526. Updated else if in OMBucketSetPropertyRequest.java
Dec 1, 2022
4193b4b
HDDS-7526. Removed unnecessary changes in OmBucketArgs.java
Dec 5, 2022
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
25 changes: 20 additions & 5 deletions hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
private boolean quotaInBytesSet = false;
private boolean quotaInNamespaceSet = false;
private DefaultReplicationConfig defaultReplicationConfig = null;
private boolean defaultReplicationConfigSet = false;
/**
* Bucket Owner Name.
*/
Expand Down Expand Up @@ -150,11 +151,20 @@ public DefaultReplicationConfig getDefaultReplicationConfig() {
return defaultReplicationConfig;
}

/**
* Returns true if defaultReplicationConfig has been set
* to a non default value.
*/
public boolean hasDefaultReplicationConfig() {

@sodonnel sodonnel Dec 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we actually need this method in this case? In #3975 the problem was a little different. The values there were longs, which in Java cannot be null and default to zero. Therefore it was impossible to tell if zero was passed or it was just the default value. The value here is an object and can be null, so maybe its enought to check it for non null?

In OMBucketSetProperty, we have:

      if (defaultReplicationConfig != null) {
        // Resetting the default replication config.
        bucketInfoBuilder.setDefaultReplicationConfig(defaultReplicationConfig);
      } else if (dbBucketInfo.getDefaultReplicationConfig() != null) {
        // Retaining existing default replication config
        bucketInfoBuilder.setDefaultReplicationConfig(
                  dbBucketInfo.getDefaultReplicationConfig());
      }

So we don't use this method there.

One thing that comes to mind - is it possible to remove a default rep config from a bucket, so it would then use the server default? Would we be able to? Is it a valid thing to be able to do?

@SaketaChalamchala SaketaChalamchala Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @sodonnel. You're right, hasDefaultReplicationConfig() is not needed in OmBucketArgs. BucketArgs object already has a hasDefaultReplicationConfig() that we can use to indicate if the replication config is set.
If the replication config needs to be "reset" i.e., removed from bucket args, would it make sense to reuse null default replication config to indicate both "to retain existing replication config" and "to reset to server default"? Would another non null indicator be useful make the distinction?

@sodonnel sodonnel Dec 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we use null for "don't overwrite" and also "clear" I am not sure how we can make the distinction.

To clear the replicationConfig, I think you will need:

  1. A new parameter on OMBucketArgs - reset or clearReplicationConfig
  2. Adjust RpcClient.setReplicationConfig, so that if a null ReplicationConfig is passed into it, it sets the new flag on OMBucketArgs.
  3. Handle the new flag in the code where it saves the bucket args.
  4. Change SetReplicationConfigHandler to either accept a "--reset" flag to indicate it need to clear the repConfig, or a new handler ResetReplicationConfigHandler to do the same, passing a null as ReplicationConfig to bucket.setReplicationConfig(...)

We can be fairly sure someone will come asking to clear the replicationConfig on a bucket some day, so it would be good to fix this. However I would do it in another Jira than this one, as I think this one is good now.

return defaultReplicationConfigSet;
}

/**
* Sets the Bucket default replication config.
*/
private void setDefaultReplicationConfig(
DefaultReplicationConfig defaultReplicationConfig) {
this.defaultReplicationConfigSet = true;
this.defaultReplicationConfig = defaultReplicationConfig;
}

Expand Down Expand Up @@ -217,6 +227,7 @@ public static class Builder {
private boolean quotaInNamespaceSet = false;
private long quotaInNamespace;
private DefaultReplicationConfig defaultReplicationConfig;
private boolean defaultReplicationConfigSet = false;
private String ownerName;
/**
* Constructs a builder.
Expand Down Expand Up @@ -265,6 +276,7 @@ public Builder setQuotaInNamespace(long quota) {

public Builder setDefaultReplicationConfig(
DefaultReplicationConfig defaultRepConfig) {
this.defaultReplicationConfigSet = true;
this.defaultReplicationConfig = defaultRepConfig;
return this;
}
Expand All @@ -284,7 +296,9 @@ public OmBucketArgs build() {
OmBucketArgs omBucketArgs =
new OmBucketArgs(volumeName, bucketName, isVersionEnabled,
storageType, metadata, ownerName);
omBucketArgs.setDefaultReplicationConfig(defaultReplicationConfig);
if (defaultReplicationConfigSet) {
omBucketArgs.setDefaultReplicationConfig(defaultReplicationConfig);
}
if (quotaInBytesSet) {
omBucketArgs.setQuotaInBytes(quotaInBytes);
}
Expand Down Expand Up @@ -316,7 +330,7 @@ public BucketArgs getProtobuf() {
quotaInNamespace > 0 || quotaInNamespace == OzoneConsts.QUOTA_RESET)) {
builder.setQuotaInNamespace(quotaInNamespace);
}
if (defaultReplicationConfig != null) {
if (defaultReplicationConfigSet && defaultReplicationConfig != null) {
builder.setDefaultReplicationConfig(defaultReplicationConfig.toProto());
}
if (ownerName != null) {
Expand All @@ -343,9 +357,10 @@ public static OmBucketArgs getFromProtobuf(BucketArgs bucketArgs) {
bucketArgs.getOwnerName() : null);
// OmBucketArgs ctor already has more arguments, so setting the default
// replication config separately.
omBucketArgs.setDefaultReplicationConfig(
new DefaultReplicationConfig(bucketArgs.getDefaultReplicationConfig()));

if (bucketArgs.hasDefaultReplicationConfig()) {
Comment thread
sodonnel marked this conversation as resolved.
omBucketArgs.setDefaultReplicationConfig(new DefaultReplicationConfig(
bucketArgs.getDefaultReplicationConfig()));
}
if (bucketArgs.hasQuotaInBytes()) {
omBucketArgs.setQuotaInBytes(bucketArgs.getQuotaInBytes());
}
Expand Down
36 changes: 36 additions & 0 deletions hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmBucketArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@

package org.apache.hadoop.ozone.om.helpers;

import org.apache.hadoop.hdds.client.DefaultReplicationConfig;
import org.apache.hadoop.hdds.client.ECReplicationConfig;
import org.junit.Assert;
import org.junit.Test;

import static org.apache.hadoop.hdds.client.ReplicationType.EC;

/**
* Tests for the OmBucketArgs class.
*/
Expand Down Expand Up @@ -58,4 +62,36 @@ public void testQuotaIsSetFlagsAreCorrectlySet() {
Assert.assertEquals(true, argsFromProto.hasQuotaInBytes());
Assert.assertEquals(true, argsFromProto.hasQuotaInNamespace());
}

@Test
public void testDefaultReplicationConfigIsSetCorrectly() {
OmBucketArgs bucketArgs = OmBucketArgs.newBuilder()
.setBucketName("bucket")
.setVolumeName("volume")
.build();

Assert.assertEquals(false, bucketArgs.hasDefaultReplicationConfig());

OmBucketArgs argsFromProto = OmBucketArgs.getFromProtobuf(
bucketArgs.getProtobuf());

Assert.assertEquals(false, argsFromProto.hasDefaultReplicationConfig());
Assert.assertEquals(null, argsFromProto.getDefaultReplicationConfig());

bucketArgs = OmBucketArgs.newBuilder()
.setBucketName("bucket")
.setVolumeName("volume")
.setDefaultReplicationConfig(new DefaultReplicationConfig(
EC, new ECReplicationConfig(3, 2)))
.build();

Assert.assertEquals(true, bucketArgs.hasDefaultReplicationConfig());

argsFromProto = OmBucketArgs.getFromProtobuf(
bucketArgs.getProtobuf());

Assert.assertEquals(true, argsFromProto.hasDefaultReplicationConfig());
Assert.assertEquals(EC,
argsFromProto.getDefaultReplicationConfig().getType());
}
}
4 changes: 4 additions & 0 deletions ...r/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
if (defaultReplicationConfig != null) {
// Resetting the default replication config.
bucketInfoBuilder.setDefaultReplicationConfig(defaultReplicationConfig);
} else if (dbBucketInfo.getDefaultReplicationConfig() != null) {
// Retaining existing default replication config
bucketInfoBuilder.setDefaultReplicationConfig(
dbBucketInfo.getDefaultReplicationConfig());
}

bucketInfoBuilder.setCreationTime(dbBucketInfo.getCreationTime());
Expand Down
88 changes: 88 additions & 0 deletions ...c/test/java/org/apache/hadoop/ozone/om/request/bucket/TestOMBucketSetPropertyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,4 +444,92 @@ public void testValidateAndUpdateCacheWithQuotaNamespaceUsed()
contains("Cannot update bucket quota. NamespaceQuota requested " +
"is less than used namespaceQuota"));
}

@Test
public void testSettingQuotaRetainsReplication() throws Exception {
String volumeName1 = UUID.randomUUID().toString();
String bucketName1 = UUID.randomUUID().toString();
String volumeName2 = UUID.randomUUID().toString();
String bucketName2 = UUID.randomUUID().toString();

/* Bucket with default replication */
OMRequestTestUtils.addVolumeAndBucketToDB(
volumeName1, bucketName1, omMetadataManager);

String bucketKey = omMetadataManager
.getBucketKey(volumeName1, bucketName1);

OmBucketInfo dbBucketInfoBefore =
omMetadataManager.getBucketTable().get(bucketKey);

/* Setting quota on a bucket with default replication */
OMRequest omRequest = createSetBucketPropertyRequest(volumeName1,
bucketName1, true, 20 * GB);

OMBucketSetPropertyRequest omBucketSetPropertyRequest =
new OMBucketSetPropertyRequest(omRequest);

OMClientResponse omClientResponse = omBucketSetPropertyRequest
.validateAndUpdateCache(ozoneManager, 1,
ozoneManagerDoubleBufferHelper);

Assert.assertEquals(true, omClientResponse.getOMResponse().getSuccess());

OmBucketInfo dbBucketInfoAfter =
omMetadataManager.getBucketTable().get(bucketKey);

Assert.assertEquals(null,
dbBucketInfoAfter.getDefaultReplicationConfig());
Assert.assertEquals(
dbBucketInfoBefore.getDefaultReplicationConfig(),
dbBucketInfoAfter.getDefaultReplicationConfig());
Assert.assertEquals(20 * GB,
dbBucketInfoAfter.getQuotaInBytes());
Assert.assertEquals(1000L,
dbBucketInfoAfter.getQuotaInNamespace());

/* Bucket with EC replication */
OmBucketInfo.Builder bucketInfo = new OmBucketInfo.Builder()
.setVolumeName(volumeName2)
.setBucketName(bucketName2)
.setDefaultReplicationConfig(new DefaultReplicationConfig(
EC, new ECReplicationConfig(3, 2)));

OMRequestTestUtils.addVolumeToDB(volumeName2, omMetadataManager);
OMRequestTestUtils.addBucketToDB(omMetadataManager, bucketInfo);

bucketKey = omMetadataManager
.getBucketKey(volumeName2, bucketName2);
dbBucketInfoBefore =
omMetadataManager.getBucketTable().get(bucketKey);

/* Setting quota on a bucket with non-default EC replication */
omRequest = createSetBucketPropertyRequest(volumeName2,
bucketName2, true, 20 * GB);

omBucketSetPropertyRequest =
new OMBucketSetPropertyRequest(omRequest);

omClientResponse = omBucketSetPropertyRequest
.validateAndUpdateCache(ozoneManager, 1,
ozoneManagerDoubleBufferHelper);

Assert.assertEquals(true, omClientResponse.getOMResponse().getSuccess());

dbBucketInfoAfter =
omMetadataManager.getBucketTable().get(bucketKey);

Assert.assertEquals(EC,
dbBucketInfoAfter.getDefaultReplicationConfig().getType());
Assert.assertEquals(
dbBucketInfoBefore.getDefaultReplicationConfig().getType(),
dbBucketInfoAfter.getDefaultReplicationConfig().getType());
Assert.assertEquals(
dbBucketInfoBefore.getDefaultReplicationConfig().getFactor(),
dbBucketInfoAfter.getDefaultReplicationConfig().getFactor());
Assert.assertEquals(20 * GB,
dbBucketInfoAfter.getQuotaInBytes());
Assert.assertEquals(1000L,
dbBucketInfoAfter.getQuotaInNamespace());
}
}