Merge pull request #3696 from rengolin/dfs-remove

HPCC-8279 Merge removeEntry and removePhysical

Reviewed-By: Jake Smith <jake.smith@lexisnexis.com>
Reviewed-By: Gavin Halliday <gavin.halliday@lexisnexis.com>
Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

common/workunit/workunit.cpp

@@ -5581,7 +5581,7 @@ void CLocalWorkUnit::deleteTempFiles(const char *graph, bool deleteOwned, bool d
         {
             const char *name = file.queryProp("@name");
             LOG(MCdebugProgress, unknownJob, "Removing workunit file %s from DFS", name);
-            queryDistributedFileDirectory().removePhysical(name, queryUserDescriptor());
+            queryDistributedFileDirectory().removeEntry(name, queryUserDescriptor());
             toRemove.append(file);
         }
     }
@@ -5689,7 +5689,7 @@ void CLocalWorkUnit::releaseFile(const char *fileName)
             files->removeTree(file);
             if (!name.isEmpty()&&(1 == usageCount))
             {
-                if (queryDistributedFileDirectory().removePhysical(fileName, queryUserDescriptor()))
+                if (queryDistributedFileDirectory().removeEntry(fileName, queryUserDescriptor()))
                     LOG(MCdebugProgress, unknownJob, "Removed (released) file %s from DFS", name.get());
             }
         }

dali/base/dadfs.cpp

@@ -914,8 +914,7 @@ public:
     bool existsPhysical(const char *_logicalname,IUserDescriptor *user);
 
     void addEntry(CDfsLogicalFileName &lfn,IPropertyTree *root,bool superfile, bool ignoreexists);
-    bool removeEntry(const char *_logicalname,IUserDescriptor *user, unsigned timeoutms=INFINITE, IDistributedFileTransaction *transaction=NULL);
-    bool removePhysical(const char *_logicalname,IUserDescriptor *user,const char *cluster=NULL,unsigned timeoutms=INFINITE,IDistributedFileTransaction *transaction=NULL);
+    bool removeEntry(const char *name, IUserDescriptor *user, IDistributedFileTransaction *transaction=NULL, const char *cluster=NULL, unsigned timeoutms=INFINITE);
     void renamePhysical(const char *oldname,const char *newname,IUserDescriptor *user,IDistributedFileTransaction *transaction);
     void removeEmptyScope(const char *name);
 
@@ -1403,9 +1402,9 @@ public:
         return udesc;
     }
 
-    bool addDelayedDelete(const char *lfn,bool remphys,IUserDescriptor *user,unsigned timeoutms,const char*cluster)
+    bool addDelayedDelete(const char *lfn,bool remphys,const char*cluster,unsigned timeoutms)
     {
-        delayeddelete.append(*new CDelayedDelete(lfn,remphys,user,timeoutms,cluster));
+        delayeddelete.append(*new CDelayedDelete(lfn,remphys,udesc,timeoutms,cluster));
         return true;
     }
     
@@ -3252,12 +3251,13 @@ public:
         root->serialize(mb);
         conn.clear();
         root.setown(createPTree(mb));
-        StringAttr lname(logicalName.get());
+        CDfsLogicalFileName lname;
+        lname.set(logicalName);
         logicalName.clear();
 #ifdef EXTRA_LOGGING
         LOGPTREE("CDistributedFile::detach root.2",root);
 #endif
-        parent->removeEntry(lname.get(),udesc, timeoutms);
+        parent->doRemoveEntry(lname,udesc,false,timeoutms);
     }
 
     bool removePhysicalPartFiles(const char *cluster,IMultiException *mexcept)
@@ -4691,9 +4691,10 @@ public:
         root.clear();
         conn.clear();
         root.setown(createPTree(mb));
-        StringAttr lname(logicalName.get());
+        CDfsLogicalFileName lname;
+        lname.set(logicalName);
         logicalName.clear();
-        parent->removeEntry(lname.get(),udesc, timeoutms);
+        parent->doRemoveEntry(lname,udesc,false,timeoutms);
     }
 
     bool removePhysicalPartFiles(const char *clustername,IMultiException *mexcept)
@@ -5118,7 +5119,7 @@ private:
                     bool done;
                     CDfsLogicalFileName dlfn;
                     dlfn.set(subnames.item(i).text.get());
-                    if (!transaction||!delayed||!transaction->addDelayedDelete(dlfn.get(),remphys,udesc)) {
+                    if (!transaction||!delayed||!transaction->addDelayedDelete(dlfn.get(),remphys)) {
                         if (remphys) 
                             done = parent->doRemovePhysical(dlfn,NULL,NULL,udesc,true);
                         else {
@@ -6978,7 +6979,7 @@ bool CDistributedFileDirectory::doRemoveEntry(CDfsLogicalFileName &dlfn,IUserDes
 }
 
 
-bool CDistributedFileDirectory::removeEntry(const char *name,IUserDescriptor *user, unsigned timeoutms, IDistributedFileTransaction *transaction)
+bool CDistributedFileDirectory::removeEntry(const char *name, IUserDescriptor *user, IDistributedFileTransaction *transaction, const char *cluster, unsigned timeoutms)
 {
     CDfsLogicalFileName logicalname;
     logicalname.set(name);
@@ -6993,8 +6994,16 @@ bool CDistributedFileDirectory::removeEntry(const char *name,IUserDescriptor *us
         localtrans.setown(new CDistributedFileTransaction(user));
     }
 
+    bool remphys = true;
+    Owned<IDistributedSuperFile> temp = localtrans->lookupSuperFileCached(name, timeoutms);
+    if (temp)
+    {
+        remphys = false; // Super-files are not physical entities
+        temp.clear();
+    }
+
     // action is owned by transaction (acquired on CDFAction's c-tor) so don't unlink or delete!
-    localtrans->addDelayedDelete(logicalname.get(), false, user, timeoutms);
+    localtrans->addDelayedDelete(logicalname.get(), remphys, cluster, timeoutms);
 
     try
     {
@@ -7065,41 +7074,6 @@ bool CDistributedFileDirectory::doRemovePhysical(CDfsLogicalFileName &dlfn,const
     }
     return true;
 }
-
-bool CDistributedFileDirectory::removePhysical(const char *_logicalname,IUserDescriptor *user,const char *cluster,unsigned timeoutms,IDistributedFileTransaction *transaction)
-{
-    CDfsLogicalFileName logicalname;
-    logicalname.set(_logicalname);
-    checkLogicalName(logicalname,user,true,true,false,"delete physical");
-
-    // Create a local transaction that will be destroyed (but never touch the external transaction)
-    Linked<IDistributedFileTransaction> localtrans;
-    if (transaction) {
-        localtrans.set(transaction);
-    } else {
-        // TODO: Make it explicit in the API that a transaction is required
-        localtrans.setown(new CDistributedFileTransaction(user));
-    }
-
-    // action is owned by transaction (acquired on CDFAction's c-tor) so don't unlink or delete!
-    localtrans->addDelayedDelete(logicalname.get(), true, user, timeoutms, cluster);
-
-    try
-    {
-        localtrans->autoCommit();
-    }
-    catch (IException *e)
-    {
-        // TODO: Transform removePhysical into void
-        StringBuffer msg;
-        e->errorMessage(msg);
-        ERRLOG("Error while deleting %s: %s", logicalname.get(), msg.str());
-        e->Release();
-        return false;
-    }
-    return true;
-}
-
     
 void CDistributedFileDirectory::renamePhysical(const char *oldname,const char *newname,IUserDescriptor *user,IDistributedFileTransaction *transaction)
 {
@@ -9770,7 +9744,7 @@ void CDistributedFileDirectory::promoteSuperFiles(unsigned numsf,const char **sf
     // MORE - once deletion of logic files are also in transaction we can move this up (and allow promote within transactions)
     if (delsub) {
         ForEachItemIn(j,outunlinked) 
-            removePhysical(outunlinked.item(j),user,NULL,timeout,transaction);
+            removeEntry(outunlinked.item(j),user,transaction,NULL,timeout);
     }
 }
 

dali/base/dadfs.hpp

@@ -185,7 +185,7 @@ interface IDistributedFileTransaction: extends IInterface
     virtual IDistributedSuperFile *lookupSuperFile(const char *slfn,unsigned timeout=INFINITE)=0;
     virtual IDistributedSuperFile *lookupSuperFileCached(const char *slfn,unsigned timeout=INFINITE)=0;
     virtual IUserDescriptor *queryUser()=0;
-    virtual bool addDelayedDelete(const char *lfn,bool remphys,IUserDescriptor *user,unsigned timeoutms=INFINITE,const char*cluster=NULL)=0; // used internally to delay deletes untill commit
+    virtual bool addDelayedDelete(const char *lfn,bool remphys,const char*cluster=NULL,unsigned timeoutms=INFINITE)=0; // used internally to delay deletes untill commit
     virtual void addAction(CDFAction *action)=0; // internal
     virtual void addFile(IDistributedFile *file)=0; // TODO: avoid this being necessary
     virtual void clearFiles()=0; // internal
@@ -457,8 +457,7 @@ interface IDistributedFileDirectory: extends IInterface
 
     virtual IDFScopeIterator *getScopeIterator(IUserDescriptor *user, const char *subscope=NULL,bool recursive=true,bool includeempty=false)=0;
 
-    virtual bool removeEntry(const char *name,IUserDescriptor *user, unsigned timeoutms=INFINITE, IDistributedFileTransaction *transaction=NULL) = 0;  // equivalent to lookup/detach/release
-    virtual bool removePhysical(const char *name,IUserDescriptor *user,const char *cluster=NULL,unsigned timeoutms=INFINITE,IDistributedFileTransaction *transaction=NULL) = 0;                           // removes the physical parts as well as entry
+    virtual bool removeEntry(const char *name, IUserDescriptor *user, IDistributedFileTransaction *transaction=NULL, const char *cluster=NULL, unsigned timeoutms=INFINITE) = 0;
     virtual void renamePhysical(const char *oldname,const char *newname,IUserDescriptor *user,IDistributedFileTransaction *transaction) = 0;                         // renames the physical parts as well as entry
     virtual void removeEmptyScope(const char *scope) = 0;   // does nothing if called on non-empty scope
     

dali/dfu/dfurun.cpp

@@ -1244,7 +1244,7 @@ public:
                                     oldfile.clear();
                                     if (!options->getOverwrite())
                                         throw MakeStringException(-1,"Destination file %s already exists and overwrite not specified",tmp.str());
-                                    fdir.removePhysical(tmp.str(),userdesc,NULL,NULL);
+                                    fdir.removeEntry(tmp.str(),userdesc);
                                 }
                             }
                             StringBuffer jobname;
@@ -1405,7 +1405,7 @@ public:
                         if (options->getNoDelete())
                             fdir.removeEntry(tmp.str(),userdesc);
                         else
-                            fdir.removePhysical(tmp.str(),userdesc,NULL,NULL);
+                            fdir.removeEntry(tmp.str(),userdesc);
                         Audit("REMOVE",userdesc,tmp.clear(),NULL);
                         runningconn.clear();
                     }

dali/regress/daregdfs.cpp

@@ -98,7 +98,7 @@ void testMultiCluster()
     for (i=0;i<file->numClusters();i++)
         PROGLOG("cluster[%d] = %s",i,file->getClusterName(i,name.clear()).str());
     file.clear();
-    queryDistributedFileDirectory().removePhysical("test::testfile1@testgrp2",UNKNOWN_USER);
+    queryDistributedFileDirectory().removeEntry("test::testfile1@testgrp2",UNKNOWN_USER);
     file.setown(queryDistributedFileDirectory().lookup("test::testfile1",UNKNOWN_USER));
     for (i=0;i<file->numClusters();i++)
         PROGLOG("cluster[%d] = %s",i,file->getClusterName(i,name.clear()).str());

dali/regress/daregress.cpp

@@ -328,7 +328,7 @@ static bool setupDFS(const char *scope, unsigned supersToDel=3, unsigned subsToC
         sub.append("::").append(name);
 
         // Remove first
-        if (dir.exists(sub.str(),user,true,false) && !dir.removePhysical(sub.str(), user)) {
+        if (dir.exists(sub.str(),user,true,false) && !dir.removeEntry(sub.str(), user)) {
             ERROR1("Can't remove %s", sub.str());
             return false;
         }
@@ -577,14 +577,10 @@ static void testDFS()
     t = 37;
     for (i=0;i<100;i++) {
         s.clear().append("daregress::test").append(t);
-        if (i%1) {
-            Owned<IDistributedFile> dfile = dir.lookup(s.str(),user);
-            if (!dfile)
-                ERROR1("Could not find %s",s.str());
-            dfile->detach();
-        }
-        else 
-            dir.removeEntry(s.str(), user);
+        Owned<IDistributedFile> dfile = dir.lookup(s.str(),user);
+        if (!dfile)
+            ERROR1("Could not find %s",s.str());
+        dfile->detach();
         t = (t+37)%100; 
     }
     printf("DFile removal complete\n");
@@ -927,7 +923,7 @@ static void testDFSDel()
 
     printf("Removing 'regress::del::sub2 - rollback\n");
     transaction->start();
-    dir.removeEntry("regress::del::sub2", user, INFINITE, transaction);
+    dir.removeEntry("regress::del::sub2", user, transaction);
     transaction->rollback();
 
     if (!dir.exists("regress::del::sub2", user, true, false)) {
@@ -937,7 +933,7 @@ static void testDFSDel()
 
     printf("Removing 'regress::del::sub2 - commit\n");
     transaction->start();
-    dir.removeEntry("regress::del::sub2", user, INFINITE, transaction);
+    dir.removeEntry("regress::del::sub2", user, transaction);
     transaction->commit();
 
     if (dir.exists("regress::del::sub2", user, true, false)) {
@@ -948,7 +944,7 @@ static void testDFSDel()
     // Physical Remove
     printf("Physically removing 'regress::del::sub3 - rollback\n");
     transaction->start();
-    dir.removeEntry("regress::del::sub3", user, INFINITE, transaction);
+    dir.removeEntry("regress::del::sub3", user, transaction);
     transaction->rollback();
 
     if (!dir.exists("regress::del::sub3", user, true, false)) {
@@ -958,7 +954,7 @@ static void testDFSDel()
 
     printf("Physically removing 'regress::del::sub3 - commit\n");
     transaction->start();
-    dir.removeEntry("regress::del::sub3", user, INFINITE, transaction);
+    dir.removeEntry("regress::del::sub3", user, transaction);
     transaction->commit();
 
     if (dir.exists("regress::del::sub3", user, true, false)) {
@@ -971,11 +967,11 @@ static void testDFSRename()
 {
     Owned<IDistributedFileTransaction> transaction = createDistributedFileTransaction(user); // disabled, auto-commit
 
-    if (dir.exists("regress::rename::other1",user,false,false) && !dir.removePhysical("regress::rename::other1", user)) {
+    if (dir.exists("regress::rename::other1",user,false,false) && !dir.removeEntry("regress::rename::other1", user)) {
         ERROR("Can't remove 'regress::rename::other1'");
         return;
     }
-    if (dir.exists("regress::rename::other2",user,false,false) && !dir.removePhysical("regress::rename::other2", user)) {
+    if (dir.exists("regress::rename::other2",user,false,false) && !dir.removeEntry("regress::rename::other2", user)) {
         ERROR("Can't remove 'regress::rename::other2'");
         return;
     }

dali/sasha/saxref.cpp

@@ -2344,7 +2344,7 @@ public:
             const char *lfn = expirylist.item(i);
             PROGLOG(LOGPFX2 "Deleting %s",lfn);
             try {
-                queryDistributedFileDirectory().removePhysical(lfn,UNKNOWN_USER);//MORE:Pass IUserDescriptor
+                queryDistributedFileDirectory().removeEntry(lfn,UNKNOWN_USER);//MORE:Pass IUserDescriptor
             }
             catch (IException *e) { // may want to just detach if fails
                 StringBuffer s;

esp/services/ws_dfu/ws_dfuService.cpp

@@ -1250,7 +1250,7 @@ bool CWsDfuEx::DFUDeleteFiles(IEspContext &context, IEspDFUArrayActionRequest &r
                 else
                 {
                     df.clear(); 
-                    deleted = queryDistributedFileDirectory().removeEntry(logicalFileName.str(),userdesc, REMOVE_FILE_SDS_CONNECT_TIMEOUT); // this can remove clusters also
+                    deleted = queryDistributedFileDirectory().removeEntry(logicalFileName.str(), userdesc, NULL, NULL, REMOVE_FILE_SDS_CONNECT_TIMEOUT); // this can remove clusters also
                     if (deleted)
                         returnStr.appendf("<Message><Value>Detached File %s</Value></Message>", logicalFileName.str());
                 }

plugins/fileservices/fileservices.cpp

@@ -349,9 +349,9 @@ FILESERVICES_API void FILESERVICES_CALL fsDeleteLogicalFile(ICodeContext *ctx, c
     Linked<IUserDescriptor> udesc = ctx->queryUserDescriptor();
     StringBuffer uname;
     PrintLog("Deleting NS logical file %s for user %s", lfn.str(),udesc?udesc->getUserName(uname).str():"");
-    if (queryDistributedFileDirectory().removePhysical(lfn.str(),udesc,NULL,INFINITE,transaction))
+    if (queryDistributedFileDirectory().removeEntry(lfn.str(),udesc,transaction))
     {
-        StringBuffer s("DeleteLogicalFile ('");         // ** TBD use removephysical (handles cluster)
+        StringBuffer s("DeleteLogicalFile ('");
         s.append(lfn);
         if (transaction->active())
             s.append("') added to transaction");
@@ -361,7 +361,8 @@ FILESERVICES_API void FILESERVICES_CALL fsDeleteLogicalFile(ICodeContext *ctx, c
         AuditMessage(ctx,"DeleteLogicalFile",lfn.str());
 
     }
-    else if (!ifexists) {
+    else if (!ifexists)
+    {
         throw MakeStringException(0, "Could not delete file %s", lfn.str());
     }
 }