HDDS-6387. [Multi-Tenant] Refactor OMMultiTenantManager and OMTenantRequestHelper #3264
Conversation
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the updates @aswinshakil. Mostly LGTM with some minor comments in line.
...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Show resolved
Hide resolved
...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the updates @aswinshakil. Just some minor things about the new isEmpty check otherwise this looks good to me.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManager.java
Show resolved
Hide resolved
| // This helper function is a placeholder for the isTenantEmpty check, | ||
| // once tenantCache/Ranger is fixed this will be removed. | ||
| if (tenantCache.containsKey(tenantId) && | ||
| !tenantCache.get(tenantId).isTenantEmpty()) { |
There was a problem hiding this comment.
The tenantCache is supposed to have all the user data for each tenant. I don't think we need to iterate the DB as well. If the tenant is not in the cache, we can throw an IOException like listUsersInTenant does, since this would indicate a bug in our OM code.
There was a problem hiding this comment.
The caller of isTenantEmpty already throws an OMException when we return false. Should we explicitly throw an IOException here as well? cc: @errose28
There was a problem hiding this comment.
I think so, because they are two different errors. If this method returns false and the caller throws OMException, it is because the client tried to delete a non-empty tenant. If this method is called with a tenant that does not exist, there is a bug in our code and neither the true or false responses make sense.
This assumes the caller already checked the validity of the tenant it got from the client though. If this method is expected to do that as well, we can through an OMException with TENANT_NOT_FOUND return code from this method instead.
smengcl
changed the title
What changes were proposed in this pull request?
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6387
How was this patch tested?
Tested using CI tests.