smengcl edited a comment on pull request #2857: URL: https://github.com/apache/ozone/pull/2857#issuecomment-977343073
> Thanks for working on this @smengcl. Large PR so lots of comments : ) At a high level there are a few things I'd like to mention: > > 1. I didn't see much testing of tenant delete beyond just that the command succeeded. Let me know if I missed something, but it looks like we need more in depth testing of the command to check that volume is removed, remaining tenant state is cleared up, etc. > 2. I think we should consider the ref count idea a bit more carefully. Right now nothing decrements the count, so if someone else uses it for something and does the same thing we did the volume will be stuck forever. Decrementing the count for us would imply an alternate tenant delete variation which deletes the tenant state but not the volume, decrementing the count. I think this might be a good thing to have but we should discuss. Thanks @errose28 for the review. 1. There is only one negative test case in the integration test, that checks the tenant can't be removed if there are accessIds assigned: https://github.com/apache/ozone/blob/3d689c924ae27d855f818be9c3f90a000674e81f/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java#L563-L568 And robot test: https://github.com/apache/ozone/pull/2857/files#diff-4ee40ef9539d7c861f0deef0d3d9ef8e7a131c2cf44e6f5c7892677f9e8ac56fR63-R65 No tests for volume emptiness / volume existence negative tests have been added yet. 2. Yes. In an earlier unpublished iteration I do decrease the volume refCount, but that logic was later replaced by the thrown exception. Since how `DeleteTenantRequest` should behave when `refCount > 1L` is unclear here. (Should we just keep the volume and only decrease the refCount? Probably no, because the Ranger policies will be removed regardless that renders the volume inaccessible to all users but cluster admins). So the assumption made here is that the volume `refCount` MUST be `<=1` (or == 1 to be exact) to pass this check. -- Essentially all other new features in the future that potentially increment the volume `refCount` must be disable on this volume first. -- 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]
