errose28 commented on a change in pull request #2857:
URL: https://github.com/apache/ozone/pull/2857#discussion_r758837640
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
##########
@@ -82,8 +94,36 @@ private OmVolumeArgs(String adminName, String ownerName,
String volume,
this.modificationTime = modificationTime;
this.objectID = objectID;
this.updateID = updateID;
+ this.refCount = refCount;
+ }
+
+ public long getRefCount() {
+ Preconditions.checkState(refCount >= 0, "refCount should not be negative");
Review comment:
This check should be moved to the decrement operation, where the
refCount value actually changes. Otherwise we will not know where the actual
error occurred.
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java
##########
@@ -84,6 +97,14 @@ private OmVolumeArgs(String adminName, String ownerName,
String volume,
this.updateID = updateID;
}
+ public long getRefCount() {
+ assert (refCount >= 0);
+ return refCount;
+ }
+
+ public void setRefCount(long refCount) {
Review comment:
Is there any reason to leave the setRefCount here? Still seems error
prone, lots of people don't read javadocs before calling things : ) If we have
a use case now we can leave it but this doesn't seem like a good thing to put
in "just in case".
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneVolume.java
##########
@@ -444,6 +471,14 @@ public void deleteBucket(String bucketName) throws
IOException {
proxy.deleteBucket(name, bucketName);
}
+ public long getRefCount() {
Review comment:
The getter is probably ok, but we should be able to get rid of the
setter without findbugs complaining because the new constructor initializes it.
Probably also a good idea to have the old constructor also set it to 0 so there
is no confusion about what its default value should be.
##########
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -668,29 +670,63 @@ public void revokeS3Secret(String kerberosID) throws
IOException {
* {@inheritDoc}
*/
@Override
- public void createTenant(String tenantName) throws IOException {
- Preconditions.checkArgument(Strings.isNotBlank(tenantName),
+ public void createTenant(String tenantId) throws IOException {
+ Preconditions.checkArgument(Strings.isNotBlank(tenantId),
"tenantName cannot be null or empty.");
- ozoneManagerClient.createTenant(tenantName);
+ ozoneManagerClient.createTenant(tenantId);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public void createTenant(String tenantId, OmTenantArgs omTenantArgs)
Review comment:
Neither of them are protobuf classes, but the convention for bucket and
volume creation is to not use the same class for client configuration and OM
representation. I think we should be following the same pattern as existing
requests like bucket and volume create here.
--
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]
