HDDS-13940. Make OmVolumeArgs owner/timestamps/quotas immutable

[adoroszlai]

What changes were proposed in this pull request?

Remove mutators for ownerName, quota*, *Time, refCount from OmVolumeArgs, make these fields final.

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

How was this patch tested?

https://github.com/adoroszlai/ozone/actions/runs/19409897987

[swamirishi]

@yandrey321 Can you please take a look at this change

[yandrey321]

LGTM

[sodonnel]

Code change looks fine, I just have a question.

OMVolumeArgs, seems to be a kind of persistent object that describes a volume in OM. I cannot see an OMVolumeInfo class. The only place the reference count seems to get incremented and decremented is in OmTenantCreateRequest, which I guess is a rare operation, so performance is likely no concern here.

Are we planning to make similar changes to bucket objects too? For example OmBucketInfo, which has incrUsedBytes etc. I would be a little concerned about GC churn for a highly mutated object like OmBucketInfo where the original object and the builder would be disposed of for each mutation.

[adoroszlai]

Are we planning to make similar changes to bucket objects too?

Yes.

For example OmBucketInfo, which has incrUsedBytes etc. I would be a little concerned about GC churn for a highly mutated object like OmBucketInfo where the original object and the builder would be disposed of for each mutation.

This already exists, because OM creates a copy of OmBucketInfo for the response. Example:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

Lines 406 to 408 in d7e6878

	omClientResponse = new OMKeyCommitResponse(omResponse.build(),
	omKeyInfo, dbOzoneKey, dbOpenKey, omBucketInfo.copyObject(),
	oldKeyVersionsToDeleteMap, isHSync, newOpenKeyInfo, dbOpenKeyToDeleteKey, openKeyToDelete);

By making OmBucketInfo immutable this copy operation can be removed.

[adoroszlai]

Converted to draft because conflicts will need to be resolved after #9327.

[sodonnel]

By making OmBucketInfo immutable this copy operation can be removed.

That is very unfortunate. BucketInfo contains these fields:

  private final String volumeName;
  private final String bucketName;
  private final CopyOnWriteArrayList<OzoneAcl> acls;
  private final boolean isVersionEnabled;
  private final StorageType storageType;
  private final long creationTime;
  private final long modificationTime;  // Probably mutable
  private final BucketEncryptionKeyInfo bekInfo;
  private final DefaultReplicationConfig defaultReplicationConfig;
  private final String sourceVolume;
  private final String sourceBucket;
  private final BucketLayout bucketLayout;
  private final String owner;

  private long usedBytes;
  private long usedNamespace;
  private final long quotaInBytes;
  private final long quotaInNamespace;
  private long snapshotUsedBytes;
  private long snapshotUsedNamespace;

Almost all of these are immutable by definition, but yet a simple increment of usedBytes requires all the fields to be copied again to update the rocksDB. Because omBucketInfo is mutated by every create and delete key, it is heavily mutated.

By making it immutable, for every create key, rather than 1 new copy of the original object, we will end up with a builder and then a new copy most likely.

It would be nice to split this object in 2, having the immutable fields in one object and the others in a smaller object, but then to write a single increment of usedBytes back to rocksDB, we need to convert the entire object back to proto and write it all back out. I'm not sure how often that happens though, as there is a cache in the picture too. However there is talk of removing the cache, at least for keys.

[adoroszlai]

It would be nice to split this object in 2, having the immutable fields in one object and the others in a smaller object, but then to write a single increment of usedBytes back to rocksDB, we need to convert the entire object back to proto and write it all back out.

That's a good idea. Ideally "usage" should be in a separate table in RocksDB.

Created HDDS-13971.

[adoroszlai]

Thanks @sodonnel, @yandrey321 for the review.