[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2629: HDDS-3371. Begin Cleanup of old write-path of key in OM

[GitBox <gi...@apache.org>]

GeorgeJahad commented on a change in pull request #2629:
URL: https://github.com/apache/ozone/pull/2629#discussion_r705909015



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -2682,27 +2682,8 @@ public void renameKey(OmKeyArgs args, String toKeyName) throws IOException {
    */
   @Override
   public void deleteKey(OmKeyArgs args) throws IOException {
-    Map<String, String> auditMap = args.toAuditMap();
-    try {
-      ResolvedBucket bucket = resolveBucketLink(args);
-      args = bucket.update(args);
-
-      if (isAclEnabled) {
-        checkAcls(ResourceType.KEY, StoreType.OZONE, ACLType.DELETE,
-            bucket.realVolume(), bucket.realBucket(), args.getKeyName());
-      }
-
-      metrics.incNumKeyDeletes();
-      keyManager.deleteKey(args);
-      AUDIT.logWriteSuccess(buildAuditMessageForSuccess(OMAction.DELETE_KEY,
-          auditMap));
-      metrics.decNumKeys();
-    } catch (Exception ex) {
-      metrics.incNumKeyDeleteFails();
-      AUDIT.logWriteFailure(buildAuditMessageForFailure(OMAction.DELETE_KEY,
-          auditMap, ex));
-      throw ex;
-    }
+    throw new UnsupportedOperationException("OzoneManager does not require " +

Review comment:
       > Just a question, instead of doing this way, is there a way we can remove these methods.
   
   I haven't looked at that much yet, because I saw that deleteKeys(), renameKeys(), setQuota() and revokeS3Secret() all use the UnsupportedOperationException approach, so I assumed that is what I was supposed to do.  I just copied the code from deleteKeys()
   
   > My question is coming from the context, as if any new write requests
   >  added to OzoneManagerProtocol, still we should add this
   >  UnsupportedOperationException?
   
   I think so, unfortunately, until we redefine the OzoneManagerProtocol.  And as @cxorm says, that is likely to be a huge patch.
   
   > If that is too much change, we can explore that in a new Jira. Lets
   >   get the work going in, as I see it is causing lot of confusion to OM
   >   developers
   
   I think that is the right decision. I think there are two separate problems
   
   1. removing the old unused methods, (and fixing the tests that use them).
   2. fixing the OzoneManagerProtocol
   
   As of right now, I think they can be handled in separate PR's.  And I think 1 is much higher priority, because as you've said, the code is causing so much confusion.  What do you think, @bharatviswa504 ?
   
   > And also in similar mannner are you also planning to delete
   >  KeyManagerImpl delete method also? (I think we can completely remove
   >  that method)
   
   Yes, but I think the most efficient way to do it is to finish each class in 'org.apached.hadoop.ozone.om' one at a time.
   
   In other words, finish removing all the methods from OzoneManager.java first.  And fix all the tests that that breaks.  Then all the methods from the KeyManager.java and fix the tests that those break, and so on.  Does that sound ok, @bharatviswa504 ?
   
   
   >  I ever used to the same throw-Exception manner and I think it doesn't solve the problem fully.
   
   So do you think it is ok to just continue on with the OM for now, and add UnsupportedOperationException's for the rest of the methods, before moving on to the other classes?  Would that cause any problems @cxorm ?
   
   




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org