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

[SaketaChalamchala]

What changes were proposed in this pull request?

Setting Quotas on an existing bucket overwrites the replication config to RATIS ONE. These are the first options for ReplicationType & ReplicationFactor enums in hdds.proto which are picked up by default.

Proposed change is similar to #3975 . Add hasDefaultReplicationConfig in OmBucketArgs.java to indicate if Replication Config is passed by the client. If not, avoid setting replication config in protobuf fields.
if replication config is not explicitly passed, retain replication config of existing bucket when setting bucket properties in OMBucketSetPropertyRequest.java

# ozone sh volume create /ecvol
22/11/29 22:52:06 INFO rpc.RpcClient: Creating Volume: ecvol, with cdpuser1 as owner and space quota set to -1 bytes, counts quota set to -1
# ozone sh bucket create /ecvol/ec1bucket
22/11/29 22:52:15 INFO rpc.RpcClient: Creating Bucket: ecvol/ec1bucket, with the Bucket Layout null, cdpuser1 as owner, Versioning false, Storage Type set to DISK and Encryption set to false
# ozone sh bucket info /ecvol/ec1bucket
# ozone sh bucket set-replication-config -t EC -r rs-3-2-1024k /ecvol/ec1bucket
# ozone sh bucket info /ecvol/ec1bucket
{
  "metadata" : { },
  "volumeName" : "ecvol",
  "name" : "ec1bucket",
  "storageType" : "DISK",
  "versioning" : false,
  "usedBytes" : 0,
  "usedNamespace" : 0,
  "creationTime" : "2022-11-29T22:52:15.599Z",
  "modificationTime" : "2022-11-29T22:52:40.943Z",
  "quotaInBytes" : -1,
  "quotaInNamespace" : -1,
  "bucketLayout" : "LEGACY",
  "link" : false,
  "replicationConfig" : {
    "data" : 3,
    "parity" : 2,
    "ecChunkSize" : 1048576,
    "codec" : "RS",
    "replicationType" : "EC",
    "requiredNodes" : 5
  }
}
[root@schal-11-1 ~]# ozone sh bucket setquota --quota=1GB --namespace-quota=1000 /ecvol/ec1bucket
[root@schal-11-1 ~]# ozone sh bucket info /ecvol/ec1bucket
{
  "metadata" : { },
  "volumeName" : "ecvol",
  "name" : "ec1bucket",
  "storageType" : "DISK",
  "versioning" : false,
  "usedBytes" : 0,
  "usedNamespace" : 0,
  "creationTime" : "2022-11-29T22:52:15.599Z",
  "modificationTime" : "2022-11-29T22:54:09.507Z",
  "quotaInBytes" : 1073741824,
  "quotaInNamespace" : 1000,
  "bucketLayout" : "LEGACY",
  "link" : false,
  "replicationConfig" : {
    "replicationFactor" : "ONE",
    "requiredNodes" : 1,
    "replicationType" : "RATIS"
  }
}
[root@schal-11-1 ~]# ozone sh bucket create ecvol/def1bucket
22/11/29 22:54:35 INFO rpc.RpcClient: Creating Bucket: ecvol/def1bucket, with the Bucket Layout null, cdpuser1 as owner, Versioning false, Storage Type set to DISK and Encryption set to false 
[root@schal-11-1 ~]# ozone sh bucket info /ecvol/def1bucket
{
  "metadata" : { },
  "volumeName" : "ecvol",
  "name" : "def1bucket",
  "storageType" : "DISK",
  "versioning" : false,
  "usedBytes" : 0,
  "usedNamespace" : 0,
  "creationTime" : "2022-11-29T22:54:35.020Z",
  "modificationTime" : "2022-11-29T22:54:35.020Z",
  "quotaInBytes" : -1,
  "quotaInNamespace" : -1,
  "bucketLayout" : "LEGACY",
  "owner" : "cdpuser1",
  "link" : false
}
[root@schal-11-1 ~]# ozone sh bucket setquota --quota=1GB --namespace-quota=1000 /ecvol/def1bucket
[root@schal-11-1 ~]# ozone sh bucket info /ecvol/def1bucket
{
  "metadata" : { },
  "volumeName" : "ecvol",
  "name" : "def1bucket",
  "storageType" : "DISK",
  "versioning" : false,
  "usedBytes" : 0,
  "usedNamespace" : 0,
  "creationTime" : "2022-11-29T22:54:35.020Z",
  "modificationTime" : "2022-11-29T22:55:25.420Z",
  "quotaInBytes" : 1073741824,
  "quotaInNamespace" : 1000,
  "bucketLayout" : "LEGACY",
  "link" : false,
  "replicationConfig" : {
    "replicationFactor" : "ONE",
    "requiredNodes" : 1,
    "replicationType" : "RATIS"
  }
}

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7526

How was this patch tested?

Added Unit Tests to verify that Replication Config is set correctly in OmBucketArgs and that existing default/non-default replication config is retained when replication config is not set explicitly by the client

[aswinshakil]

Thanks for working on this @SaketaChalamchala I have a minor comment inline.

[aswinshakil]

Nit: We can change this to else if statement.

[SaketaChalamchala]

Thanks for the review @aswinshakil . Addressed the comment.

[aswinshakil]

LGTM+1. @sodonnel Do you want to take a look at this? As it is similar to #3975

[sodonnel]

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]

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]

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.