Merge pull request #4505 from jakesmith/hpcc-9491

HPCC-9491 - Ensure lock checked in multiple cluster case

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

dali/base/dadfs.cpp

@@ -2156,6 +2156,34 @@ inline void dfCheckRoot(const char *trc,Owned<IPropertyTree> &root,IRemoteConnec
     }
 }
 
+class CFileChangeWriteLock
+{
+    Owned<IRemoteConnection> &conn;
+    Owned<IPropertyTree> &root;
+    unsigned timeoutMs, prevMode;
+public:
+    CFileChangeWriteLock(Owned<IRemoteConnection> &_conn, unsigned _timeoutMs, Owned<IPropertyTree> &_root)
+        : conn(_conn), timeoutMs(_timeoutMs), root(_root)
+    {
+        prevMode = conn->queryMode();
+        unsigned newMode = (prevMode & ~RTM_LOCKBASIC_MASK) | RTM_LOCK_WRITE;
+        conn->changeMode(RTM_LOCK_WRITE, timeoutMs);
+    }
+    ~CFileChangeWriteLock()
+    {
+        if (conn.get())
+            conn->changeMode(prevMode, timeoutMs);
+    }
+    IPropertyTree *detach(bool close)
+    {
+        Owned<IPropertyTree> detachedRoot = createPTreeFromIPT(root);
+        root.clear();
+        conn->close(close);
+        conn.clear();
+        return detachedRoot.getClear();
+    }
+};
+
 static bool setFileProtectTree(IPropertyTree &p,const char *owner, bool protect)
 {
     bool ret = false;
@@ -2296,81 +2324,10 @@ protected:
         root->removeProp("Attr");
         return NULL;
     }
-
-    // Call this function *ONLY* from detach()
-    // MORE: When the refactoring tips below get implemented, this method can go
-    // And the appropriate sub methods can be created depending whether this is
-    // a super-file or simply a file
-    void doRemoveEntry(CDfsLogicalFileName &lfn, IUserDescriptor *user, unsigned timeoutms=INFINITE)
+    void updateFS(const CDfsLogicalFileName &lfn, unsigned timeoutMs)
     {
-        StringBuffer cname;
-        lfn.getCluster(cname);
-        DfsXmlBranchKind bkind;
-        CFileConnectLock fconnlock;
-        {
-            IPropertyTree *froot=NULL;
-            if (fconnlock.initany("doRemoveEntry", lfn, bkind, true, false, false, timeoutms))
-                froot = fconnlock.queryRoot();
-            if (!froot)
-                ThrowStringException(-1, "Can't find SDS node for %s", lfn.get());
-
-            // Remove Cluster from Logical file
-            // MORE: Move this to a doRemoveCluster method
-            if (cname.length()) {
-                if (bkind==DXB_SuperFile)
-                    ThrowStringException(-1, "Trying to remove cluster %s from superfile %s",cname.str(),lfn.get());
-
-                const char *group = froot->queryProp("@group");
-                if (group&&(strcmp(group,cname.str())!=0)) {    // see if only cluster (if it is remove entire)
-                    StringBuffer query;
-                    query.appendf("Cluster[@name=\"%s\"]",cname.str());
-                    IPropertyTree *t = froot->queryPropTree(query.str());
-                    if (t) {
-                        if (froot->removeTree(t))
-                            return;
-                        else
-                            ThrowStringException(-1, "Can't remove cluster %s from %s",cname.str(),lfn.get());
-                    }
-                    else
-                        ThrowStringException(-1, "Cluster %s not present in file %s",cname.str(),lfn.get());
-                }
-            }
-
-            // Remove SuperOwners from all sub files
-            if (bkind==DXB_SuperFile) {
-                Owned<IPropertyTreeIterator> iter = froot->getElements("SubFile");
-                StringBuffer oquery;
-                oquery.append("SuperOwner[@name=\"").append(lfn.get()).append("\"]");
-                Owned<IMultiException> exceptions = MakeMultiException("CDelayedDelete::doRemoveEntry::SuperOwners");
-                ForEach(*iter) {
-                    const char *name = iter->query().queryProp("@name");
-                    if (name&&*name) {
-                        CDfsLogicalFileName subfn;
-                        subfn.set(name);
-                        CFileConnectLock fconnlockSub;
-                        DfsXmlBranchKind subbkind;
-                        // MORE: Use CDistributedSuperFile::linkSuperOwner(false) - ie. unlink
-                        if (fconnlockSub.initany("CDelayedDelete::doRemoveEntry", subfn, subbkind, false, false, false, timeoutms))
-                        {
-                            IPropertyTree *subfroot = fconnlockSub.queryRoot();
-                            if (subfroot) {
-                                if (!subfroot->removeProp(oquery.str()))
-                                    exceptions->append(*MakeStringException(-1, "CDelayedDelete::removeEntry: SubFile %s of %s not found for removal",name?name:"(NULL)",lfn.get()));
-                            }
-                        }
-                    }
-                }
-                if (exceptions->ordinality())
-                    throw exceptions.getClear();
-            }
-        }
-
-        // Remove from SDS and clean the lock
-        fconnlock.remove();
-        fconnlock.kill();
-
         // Update the file system
-        removeFileEmptyScope(lfn,timeoutms);
+        removeFileEmptyScope(lfn, timeoutMs);
         // MORE: We shouldn't have to update all relationships if we had done a better job making sure
         // that all correct relationships were properly cleaned up
         queryDistributedFileDirectory().removeAllFileRelationships(lfn.get());
@@ -2727,10 +2684,7 @@ public:
 
 class CDistributedFile: public CDistributedFileBase<IDistributedFile>
 {
-    // If detachment is logic-only (used *only* on rename)
-    bool detachLogic; // MORE: find a better way of doing this
 protected:
-    Owned<IFileDescriptor> fdesc;
     CDistributedFilePartArray parts;            // use queryParts to access
     CriticalSection sect;
     StringAttr directory;
@@ -2787,6 +2741,142 @@ protected:
                 parts.item(i1++).clearDirty();
         }
     }
+    void detach(unsigned timeoutMs=INFINITE, bool removePhysicals=true)
+    {
+        // Removes either a cluster in case of multi cluster file or the whole File entry from DFS
+
+        assert(proplockcount == 0 && "CDistributedFile detach: Some properties are still locked");
+        assertex(!isAnon()); // not attached!
+
+        if (removePhysicals)
+        {
+            // Avoid removing physically when there is no physical representation
+            if (logicalName.isMulti() || logicalName.isExternal())
+                removePhysicals = false;
+        }
+
+        StringBuffer clusterName;
+        Owned<IFileDescriptor> fileDescCopy;
+#ifdef EXTRA_LOGGING
+        PROGLOG("CDistributedFile::detach(%s)",logicalName.get());
+        LOGPTREE("CDistributedFile::detach root.1",root);
+#endif
+        {
+            CriticalBlock block(sect); // JCSMORE - not convinced this is still necessary
+            CFileChangeWriteLock writeLock(conn, timeoutMs, root);
+
+            logicalName.getCluster(clusterName);
+
+            // copy file descriptor before altered, used by physical file removal routines
+            if (removePhysicals)
+            {
+                MemoryBuffer mb;
+                Owned<IFileDescriptor> fdesc = getFileDescriptor(clusterName);
+                fdesc->serialize(mb);
+                fileDescCopy.setown(deserializeFileDescriptor(mb));
+            }
+
+            bool removeFile=true;
+            if (clusterName.length())
+            {
+                // Remove just cluster specified, unless it's the last, in which case detach below will remove File entry.
+                if (clusters.ordinality()>1)
+                {
+                    if (removeCluster(clusterName.str()))
+                        removeFile=false;
+                    else
+                        ThrowStringException(-1, "Cluster %s not present in file %s", clusterName.str(), logicalName.get());
+                }
+            }
+
+            // detach this IDistributeFile
+            root.setown(writeLock.detach(removeFile));
+            // NB: The file is now unlocked
+            if (removeFile)
+                updateFS(logicalName, timeoutMs);
+
+            logicalName.clear();
+        }
+        // NB: beyond unlock
+        if (removePhysicals)
+        {
+            CriticalBlock block(physicalChange);
+            Owned<IMultiException> exceptions = MakeMultiException("CDistributedFile::detach");
+            removePhysicalPartFiles(fileDescCopy, exceptions);
+            if (exceptions->ordinality())
+                throw exceptions.getClear();
+        }
+    }
+    bool removePhysicalPartFiles(IFileDescriptor *fileDesc, IMultiException *mexcept)
+    {
+        if (logicalName.isExternal())
+        {
+            if (logicalName.isQuery())
+                return false;
+            throw MakeStringException(-1,"cannot remove an external file (%s)",logicalName.get());
+        }
+        if (logicalName.isForeign())
+            throw MakeStringException(-1,"cannot remove a foreign file (%s)",logicalName.get());
+
+        class casyncfor: public CAsyncFor
+        {
+            IFileDescriptor *fileDesc;
+            CriticalSection errcrit;
+            IMultiException *mexcept;
+        public:
+            bool ok;
+            bool islazy;
+            casyncfor(IFileDescriptor *_fileDesc, IMultiException *_mexcept)
+            {
+                fileDesc = _fileDesc;
+                ok = true;
+                islazy = false;
+                mexcept = _mexcept;
+            }
+            void Do(unsigned i)
+            {
+                IPartDescriptor *part = fileDesc->queryPart(i);
+                unsigned nc = part->numCopies();
+                for (unsigned copy = 0; copy < nc; copy++)
+                {
+                    RemoteFilename rfn;
+                    part->getFilename(copy, rfn);
+                    Owned<IFile> partfile = createIFile(rfn);
+                    StringBuffer eps;
+                    try
+                    {
+                        unsigned start = msTick();
+                        if (!partfile->remove()&&(copy==0)&&!islazy) // only warn about missing primary files
+                            LOG(MCwarning, unknownJob, "Failed to remove file part %s from %s", partfile->queryFilename(),rfn.queryEndpoint().getUrlStr(eps).str());
+                        else
+                        {
+                            unsigned t = msTick()-start;
+                            if (t>5*1000)
+                                LOG(MCwarning, unknownJob, "Removing %s from %s took %ds", partfile->queryFilename(), rfn.queryEndpoint().getUrlStr(eps).str(), t/1000);
+                        }
+                    }
+                    catch (IException *e)
+                    {
+                        CriticalBlock block(errcrit);
+                        if (mexcept)
+                            mexcept->append(*e);
+                        else
+                        {
+                            StringBuffer s("Failed to remove file part ");
+                            s.append(partfile->queryFilename()).append(" from ");
+                            rfn.queryEndpoint().getUrlStr(s);
+                            EXCLOG(e, s.str());
+                            e->Release();
+                        }
+                        ok = false;
+                    }
+                }
+            }
+        } afor(fileDesc, mexcept);
+        afor.islazy = fileDesc->queryProperties().getPropBool("@lazy");
+        afor.For(fileDesc->numParts(),10,false,true);
+        return afor.ok;
+    }
 
 protected: friend class CDistributedFilePart;
     CDistributedFilePartArray &queryParts()
@@ -2816,7 +2906,6 @@ public:
         setClusters(fdesc);
         setPreferredClusters(_parent->defprefclusters);
         setParts(fdesc,false);
-        detachLogic = false;
         //shrinkFileTree(root); // enable when safe!
     }
 
@@ -2876,7 +2965,6 @@ public:
 #ifdef EXTRA_LOGGING
         LOGPTREE("CDistributedFile.b root.2",root);
 #endif
-        detachLogic = false;
     }
 
     void killParts()
@@ -3071,7 +3159,7 @@ public:
         saveClusters();
     }
 
-    void removeCluster(const char *clustername)
+    bool removeCluster(const char *clustername)
     {
         CClustersLockedSection cls(CDistributedFileBase<IDistributedFile>::logicalName);
         reloadClusters();
@@ -3081,7 +3169,9 @@ public:
                 throw MakeStringException(-1,"CFileClusterOwner::removeCluster cannot remove sole cluster %s",clustername);
             clusters.remove(i);
             saveClusters();
+            return true;
         }
+        return false;
     }
 
     void setPreferredClusters(const char *clusterlist)
@@ -3365,157 +3455,13 @@ public:
 #endif
     void detachLogical(unsigned timeoutms=INFINITE)
     {
-        detachLogic = true;
-        try {
-            detach(timeoutms);
-        } catch (...) {
-            detachLogic = false;
-            throw;
-        }
-        detachLogic = false;
+        detach(timeoutms, false);
     }
 
 public:
-
-    void detach(unsigned timeoutms=INFINITE)
+    virtual void detach(unsigned timeoutMs=INFINITE)
     {
-        assert(proplockcount == 0 && "CDistributedFile detach: Some properties are still locked");
-        assertex(!isAnon()); // not attached!
-
-        // If cluster name was passed via query (filename@cluster), try to find it
-        StringBuffer clustername;
-        logicalName.getCluster(clustername);
-
-        // Avoid removing physically when there is no physical representation
-        bool remphys = !detachLogic;
-        if (logicalName.isMulti() || logicalName.isExternal())
-            remphys = false;
-
-        // Clean up file and remove from SDS only if this is the last
-        // cluster it belongs to, since we should be able to still remove
-        // the other clusters' files. Or if there are no clusters.
-        if ((clustername.length()==0) /* no cluster passed - ie. detach all */
-           ||((findCluster(clustername.str())==0)&&(numClusters()==1))) /* cluster is first, and no other clusters */
-        {
-            CriticalBlock block (sect);
-            MemoryBuffer mb;
-#ifdef EXTRA_LOGGING
-            PROGLOG("CDistributedFile::detach(%s)",logicalName.get());
-            LOGPTREE("CDistributedFile::detach root.1",root);
-#endif
-            root->serialize(mb);
-            conn.clear();
-            root.setown(createPTree(mb));
-            CDfsLogicalFileName lname;
-            lname.set(logicalName);
-            logicalName.clear();
-#ifdef EXTRA_LOGGING
-            LOGPTREE("CDistributedFile::detach root.2",root);
-#endif
-            // Remove from SDS
-            doRemoveEntry(lname,udesc,timeoutms);
-            // Make sure we remove *all* physical instances
-            clustername.clear();
-        }
-
-        // Remove parts, physically
-        if (remphys) {
-            CriticalBlock block(physicalChange);
-            Owned<IMultiException> exceptions = MakeMultiException("CDistributedFile::detach");
-            removePhysicalPartFiles(clustername.str(),exceptions);
-            if (exceptions->ordinality())
-                throw exceptions.getClear();
-        }
-    }
-
-    bool removePhysicalPartFiles(const char *cluster,IMultiException *mexcept)
-    {
-        Owned<IGroup> grpfilter;
-        if (cluster&&*cluster) {
-            unsigned cn = findCluster(cluster);
-            if (cn==NotFound)
-                return false;
-            if (clusters.ordinality()==0)
-                cluster = NULL; // cannot delete last cluster
-            else
-                grpfilter.setown(clusters.getGroup(cn));
-        }
-        if (logicalName.isExternal()) {
-            if (logicalName.isQuery())
-                return false;
-            throw MakeStringException(-1,"cannot remove an external file (%s)",logicalName.get());
-        }
-        if (logicalName.isForeign())
-            throw MakeStringException(-1,"cannot remove a foreign file (%s)",logicalName.get());
-
-        unsigned width = numParts();
-        CriticalSection errcrit;
-        class casyncfor: public CAsyncFor
-        {
-            IDistributedFile *file;
-            CriticalSection &errcrit;
-            IMultiException *mexcept;
-            unsigned width;
-            IGroup *grpfilter;
-        public:
-            bool ok;
-            bool islazy;
-            casyncfor(IDistributedFile *_file,unsigned _width,IGroup *_grpfilter,IMultiException *_mexcept,CriticalSection &_errcrit)
-                : errcrit(_errcrit)
-            {
-                file = _file;
-                ok = true;
-                islazy = false;
-                mexcept = _mexcept;
-                width = _width;
-                grpfilter = _grpfilter;
-            }
-            void Do(unsigned i)
-            {
-                Owned<IDistributedFilePart> part = file->getPart(i);
-                unsigned nc = part->numCopies();
-                for (unsigned copy = 0; copy < nc; copy++)
-                {
-                    RemoteFilename rfn;
-                    part->getFilename(rfn,copy);
-                    if (grpfilter&&(grpfilter->rank(rfn.queryEndpoint())==RANK_NULL))
-                        continue;
-                    Owned<IFile> partfile = createIFile(rfn);
-                    StringBuffer eps;
-                    try
-                    {
-                        unsigned start = msTick();
-                        if (!partfile->remove()&&(copy==0)&&!islazy) // only warn about missing primary files
-                            LOG(MCwarning, unknownJob, "Failed to remove file part %s from %s", partfile->queryFilename(),rfn.queryEndpoint().getUrlStr(eps).str());
-                        else {
-                            unsigned t = msTick()-start;
-                            if (t>5*1000) 
-                                LOG(MCwarning, unknownJob, "Removing %s from %s took %ds", partfile->queryFilename(), rfn.queryEndpoint().getUrlStr(eps).str(), t/1000);
-                        }
-
-                    }
-                    catch (IException *e)
-                    {
-                        CriticalBlock block(errcrit);
-                        if (mexcept) 
-                            mexcept->append(*e);
-                        else {
-                            StringBuffer s("Failed to remove file part ");
-                            s.append(partfile->queryFilename()).append(" from ");
-                            rfn.queryEndpoint().getUrlStr(s);
-                            EXCLOG(e, s.str());
-                            e->Release();
-                        }
-                        ok = false;
-                    }
-                }
-            }
-        } afor(this,width,grpfilter,mexcept,errcrit);
-        afor.islazy = queryAttributes().getPropInt("@lazy")!=0;
-        afor.For(width,10,false,true);
-        if (cluster&&*cluster) 
-            removeCluster(cluster);
-        return afor.ok;
+        detach(timeoutMs, true);
     }
 
     bool existsPhysicalPartFiles(unsigned short port)
@@ -4388,6 +4334,37 @@ class CDistributedSuperFile: public CDistributedFileBase<IDistributedSuperFile>
 protected:
     int interleaved; // 0 not interleaved, 1 interleaved old, 2 interleaved new
 
+    void clearSuperOwners(unsigned timeoutMs)
+    {
+        Owned<IPropertyTreeIterator> iter = root->getElements("SubFile");
+        StringBuffer oquery;
+        oquery.append("SuperOwner[@name=\"").append(logicalName.get()).append("\"]");
+        Owned<IMultiException> exceptions = MakeMultiException("CDelayedDelete::doRemoveEntry::SuperOwners");
+        ForEach(*iter)
+        {
+            const char *name = iter->query().queryProp("@name");
+            if (name&&*name)
+            {
+                CDfsLogicalFileName subfn;
+                subfn.set(name);
+                CFileConnectLock fconnlockSub;
+                DfsXmlBranchKind subbkind;
+                // MORE: Use CDistributedSuperFile::linkSuperOwner(false) - ie. unlink
+                if (fconnlockSub.initany("CDelayedDelete::doRemoveEntry", subfn, subbkind, false, false, false, timeoutMs))
+                {
+                    IPropertyTree *subfroot = fconnlockSub.queryRoot();
+                    if (subfroot)
+                    {
+                        if (!subfroot->removeProp(oquery.str()))
+                            exceptions->append(*MakeStringException(-1, "CDelayedDelete::removeEntry: SubFile %s of %s not found for removal",name?name:"(NULL)", logicalName.get()));
+                    }
+                }
+            }
+        }
+        if (exceptions->ordinality())
+            throw exceptions.getClear();
+    }
+
     static StringBuffer &getSubPath(StringBuffer &path,unsigned idx)
     {
         return path.append("SubFile[@num=\"").append(idx+1).append("\"]");
@@ -4908,8 +4885,6 @@ public:
         return ret; 
     }
 
-
-
     void attach(const char *_logicalname,IUserDescriptor *user)
     {
         // will need more thought but this gives limited support for anon
@@ -4929,32 +4904,22 @@ public:
         root.setown(conn->getRoot());
     }
 
-    void detach(unsigned timeoutms=INFINITE)
+    void detach(unsigned timeoutMs=INFINITE)
     {   
         // will need more thought but this gives limited support for anon
         if (isAnon())
             return;
         assertex(conn.get()); // must be attached
-        CriticalBlock block (sect);
+        CriticalBlock block(sect);
         checkModify("CDistributedSuperFile::detach");
         subfiles.kill();    
-        MemoryBuffer mb;
-        root->serialize(mb);
-        root.clear();
-        conn.clear();
-        root.setown(createPTree(mb));
-        CDfsLogicalFileName lname;
-        lname.set(logicalName);
-        logicalName.clear();
 
         // Remove from SDS
-        doRemoveEntry(lname,udesc,timeoutms);
-    }
-
-    bool removePhysicalPartFiles(const char *clustername,IMultiException *mexcept)
-    {
-        throw MakeStringException(-1,"removePhysicalPartFiles not supported for SuperFiles");
-        return false; 
+        CFileChangeWriteLock writeLock(conn, timeoutMs, root);
+        clearSuperOwners(timeoutMs);
+        root.setown(writeLock.detach(true));
+        updateFS(logicalName, timeoutMs);
+        logicalName.clear();
     }
 
     bool existsPhysicalPartFiles(unsigned short port)
@@ -5614,14 +5579,16 @@ public:
         subfiles.item(0).addCluster(clustername,mspec);
     }
 
-    virtual void removeCluster(const char *clustername)
+    virtual bool removeCluster(const char *clustername)
     {
+        bool clusterRemoved=false;
         CriticalBlock block (sect);
         clusterscache.clear();
         ForEachItemIn(i,subfiles) {
             IDistributedFile &f=subfiles.item(i);
-            f.removeCluster(clustername);
+            clusterRemoved |= f.removeCluster(clustername);
         }       
+        return clusterRemoved;
     }
 
     void setPreferredClusters(const char *clusters)

dali/base/dadfs.hpp

@@ -241,9 +241,6 @@ interface IDistributedFile: extends IInterface
 
     virtual unsigned numCopies(unsigned partno) = 0;                            // number of copies
 
-    virtual bool removePhysicalPartFiles(const char *cluster=NULL,IMultiException *exceptions=NULL) = 0;          // removes all physical part files
-                                                                                // returns true if no major errors
-
     virtual bool existsPhysicalPartFiles(unsigned short port) = 0;              // returns true if physical patrs all exist (on primary OR secondary)
 
     virtual bool renamePhysicalPartFiles(const char *newlfn,const char *cluster=NULL,IMultiException *exceptions=NULL,const char *newbasedir=NULL) = 0;           // renames all physical part files
@@ -272,7 +269,7 @@ interface IDistributedFile: extends IInterface
     virtual void setECL(const char *ecl) = 0;
 
     virtual void addCluster(const char *clustername,const ClusterPartDiskMapSpec &mspec) = 0;
-    virtual void removeCluster(const char *clustername) = 0;    // doesn't delete parts
+    virtual bool removeCluster(const char *clustername) = 0;    // doesn't delete parts
     virtual bool checkClusterCompatible(IFileDescriptor &fdesc, StringBuffer &err) = 0;
     virtual void updatePartDiskMapping(const char *clustername,const ClusterPartDiskMapSpec &spec)=0;
 

dali/base/dafdesc.cpp

@@ -394,7 +394,7 @@ public:
             group.setown(createIGroup(grptext));
         mspec.deserialize(mb);
         mb.read(name);
-        if ((name.length()==1)&&(*name.get()=='!')) { // flag for roxie lable
+        if ((name.length()==1)&&(*name.get()=='!')) { // flag for roxie label
             mb.read(roxielabel);
             name.clear();
         }
@@ -1206,6 +1206,19 @@ class CFileDescriptor:  public CFileDescriptorBase, implements ISuperFileDescrip
         return NULL;
     }
 
+    IClusterInfo *queryCluster(const char *_clusterName)
+    {
+        StringAttr clusterName = _clusterName;
+        clusterName.toLowerCase();
+        StringBuffer name;
+        ForEachItemIn(c, clusters)
+        {
+            if (0 == strcmp(clusters.item(c).getClusterLabel(name.clear()).str(), clusterName))
+                return &clusters.item(c);
+        }
+        return NULL;
+    }
+
     void replaceClusterDir(unsigned partno,unsigned copy, StringBuffer &path)
     {
         // assumes default dir matches one of clusters
@@ -1447,6 +1460,8 @@ public:
         unsigned n = clusters.ordinality();
         pt.setPropInt("@numclusters",n);
         unsigned cn = 0;
+
+// JCSMORE - afaics, IFileDescriptor @group is no longer used
         StringBuffer grplist;
         ForEachItemIn(i1,clusters) {
             Owned<IPropertyTree> ct = createPTree("Cluster");
@@ -1467,6 +1482,8 @@ public:
             pt.setProp("@group",grplist.str());
         else
             pt.removeProp("@group");
+/// ^^
+
         n = numParts();
         pt.setPropInt("@numparts",n);
         if ((flags&IFDSF_EXCLUDE_PARTS)==0) {
@@ -1882,12 +1899,15 @@ public:
         closePending();
         unsigned done = 0;
         StringBuffer cname;
-        ForEachItemIn(i,names) {
+        ForEachItemIn(i,names)
+        {
             StringAttr name = names.item(i);
             name.toLowerCase();
-            for (unsigned j=done;j<clusters.ordinality();j++) {
+            for (unsigned j=done;j<clusters.ordinality();j++)
+            {
                 clusters.item(j).getClusterLabel(cname.clear());
-                if (strcmp(cname.str(),name)==0) {
+                if (strcmp(cname.str(),name)==0)
+                {
                     if (done!=j)
                         clusters.swap(done,j);
                     done++;
@@ -1895,11 +1915,31 @@ public:
                 }
             }
         }
-        if (exclusive) {
+        if (exclusive)
+        {
             if (!done)
                 done = 1;
+            StringAttr oldDefaultDir;
+            StringBuffer baseDir1;
             while (clusters.ordinality()>done)
+            {
+                clusters.item(clusters.ordinality()-1).getBaseDir(baseDir1.clear(),SepCharBaseOs(getPathSepChar(directory)));
+
+                // if baseDir is leading component this file's default directory..
+                if (!oldDefaultDir.length() && directory.length()>=baseDir1.length() && 0==strncmp(directory, baseDir1, baseDir1.length()) &&
+                    (directory.length()==baseDir1.length() || isPathSepChar(directory[baseDir1.length()])))
+                    oldDefaultDir.set(baseDir1.str());
                 clusters.remove(clusters.ordinality()-1);
+            }
+            if (oldDefaultDir.length() && clusters.ordinality())
+            {
+                StringBuffer baseDir2;
+                clusters.item(0).getBaseDir(baseDir2.clear(), SepCharBaseOs(getPathSepChar(directory)));
+                StringBuffer newDir(baseDir2.str());
+                if (directory.length()>oldDefaultDir.length())
+                    newDir.append(directory.get()+oldDefaultDir.length());
+                directory.set(newDir.str());
+            }
         }
     }
 

dali/base/dafdesc.hpp

@@ -213,8 +213,9 @@ if endCluster is not called it will assume only one cluster and not replicated
 
     virtual IGroup *getGroup() = 0;                                             // will be for first cluster
 
-    virtual unsigned numClusters()=0;
-    virtual ClusterPartDiskMapSpec &queryPartDiskMapping(unsigned clusternum)=0;
+    virtual unsigned numClusters() = 0;
+    virtual IClusterInfo *queryCluster(const char *clusterName) = 0;
+    virtual ClusterPartDiskMapSpec &queryPartDiskMapping(unsigned clusternum) = 0;
     virtual IGroup *queryClusterGroup(unsigned clusternum) = 0;                     // returns group for cluster if known
     virtual void setClusterGroup(unsigned clusternum,IGroup *grp) = 0;              // sets group for cluster
     virtual StringBuffer &getClusterGroupName(unsigned clusternum,StringBuffer &ret,IGroupResolver *resolver=NULL) = 0;                 // returns group name of cluster (if set)

dali/base/dautils.cpp

@@ -722,9 +722,12 @@ const char *CDfsLogicalFileName::get(bool removeforeign) const
     return ret;
 }
 
-StringBuffer &CDfsLogicalFileName::get(StringBuffer &str,bool removeforeign) const
+StringBuffer &CDfsLogicalFileName::get(StringBuffer &str, bool removeforeign, bool withCluster) const
 {
-    return str.append(get(removeforeign));
+    str.append(get(removeforeign));
+    if (withCluster && cluster.length())
+        str.append("@").append(cluster);
+    return str;
 }
 
 

dali/base/dautils.hpp

@@ -105,7 +105,7 @@ public:
     StringBuffer &getCluster(StringBuffer &cname) const { return cname.append(cluster); }
 
     const char *get(bool removeforeign=false) const;
-    StringBuffer &get(StringBuffer &str,bool removeforeign=false) const;
+    StringBuffer &get(StringBuffer &str,bool removeforeign=false, bool withCluster=false) const;
     const char *queryTail() const;
     StringBuffer &getTail(StringBuffer &buf) const;
 

dali/dfu/dfurun.cpp

@@ -794,7 +794,6 @@ public:
 //                      destination->setClusterPartDefaultBaseDir(tmp.str(),basedir);
                 }
             }
-            options->setNoDelete(ctx.superoptions->getNoDelete());
             options->setNoSplit(ctx.superoptions->getNoSplit());
             options->setOverwrite(ctx.superoptions->getOverwrite());
             options->setReplicate(ctx.superoptions->getReplicate());
@@ -869,9 +868,8 @@ public:
         if (dfile) {
             if (!ctx.superoptions->getOverwrite()) 
                 throw MakeStringException(-1,"Destination file %s already exists",dlfn.get());
-            if (dfile->querySuperFile()) 
-                dfile->detach();
-            else {
+            if (!dfile->querySuperFile())
+            {
                 if (ctx.superoptions->getIfModified()&&
                     (ftree->hasProp("Attr/@fileCrc")&&ftree->getPropInt64("Attr/@size")&&
                     ((unsigned)ftree->getPropInt64("Attr/@fileCrc")==(unsigned)dfile->queryAttributes().getPropInt64("@fileCrc"))&&
@@ -879,9 +877,8 @@ public:
                     PROGLOG("File copy of %s not done as file unchanged",srclfn);
                     return;
                 }
-                dfile->detach();
-                dfile->removePhysicalPartFiles(NULL,NULL);
             }
+            dfile->detach();
             dfile.clear();
         }
         if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_File))==0) {
@@ -1403,10 +1400,7 @@ public:
                     source->getLogicalName(tmp.clear());
                     if (tmp.length()) {
                         runningconn.setown(setRunning(runningpath.str()));;
-                        if (options->getNoDelete())
-                            fdir.removeEntry(tmp.str(),userdesc);
-                        else
-                            fdir.removeEntry(tmp.str(),userdesc);
+                        fdir.removeEntry(tmp.str(),userdesc);
                         Audit("REMOVE",userdesc,tmp.clear(),NULL);
                         runningconn.clear();
                     }

dali/dfu/dfuutil.cpp

@@ -616,12 +616,7 @@ public:
         if (dfile) {
             if (!overwrite)
                 throw MakeStringException(-1,"Destination file %s already exists",dlfn.get());
-            if (dfile->querySuperFile())
-                dfile->detach();
-            else {
-                dfile->detach();
-                dfile->removePhysicalPartFiles(NULL,NULL);
-            }
+            dfile->detach();
             dfile.clear();
         }
         if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_File))==0) {
@@ -692,12 +687,7 @@ public:
         if (dfile) {
             if (!overwrite)
                 throw MakeStringException(-1,"Destination file %s already exists",dlfn.get());
-            if (dfile->querySuperFile())
-                dfile->detach();
-            else {
-                dfile->detach();
-                dfile->removePhysicalPartFiles(NULL,NULL);
-            }
+            dfile->detach();
             dfile.clear();
         }
         cloneSubFile(ftree,dlfn.get(),srcdali);
@@ -869,56 +859,63 @@ public:
         SocketEndpoint daliep = srcdali;
         CDfsLogicalFileName slfn;
         slfn.set(srclfn);
-        if (slfn.isForeign()) { // trying to confuse me
+        if (slfn.isForeign()) // trying to confuse me
+        {
             slfn.getEp(daliep);
             slfn.clearForeign();
         }
         if (daliep.port==0)
             daliep.port= DALI_SERVER_PORT;
         Owned<INode> srcnode = createINode(daliep);
-        if (queryCoven().inCoven(srcnode)) {
+        if (queryCoven().inCoven(srcnode))
+        {
             // if dali is local and filenames same
             CDfsLogicalFileName dlfn;
             dlfn.set(lfn);
-            if (strcmp(slfn.get(),dlfn.get())==0) {
+            if (strcmp(slfn.get(),dlfn.get())==0)
+            {
                 PROGLOG("File copy of %s not done as file local",srclfn);
                 return;
             }
         }
         Owned<IPropertyTree> ftree = queryDistributedFileDirectory().getFileTree(srclfn,srcuser,srcnode, FOREIGN_DALI_TIMEOUT, false);
-        if (!ftree.get()) {
+        if (!ftree.get())
+        {
             StringBuffer s;
             throw MakeStringException(-1,"Source file %s could not be found in Dali %s",srclfn,daliep.getUrlStr(s).str());
         }
         // first see if target exists (and remove if does and overwrite specified)
         Owned<IDistributedFile> dfile = queryDistributedFileDirectory().lookup(lfn,user,true);
-        if (dfile) {
+        if (dfile)
+        {
             if (!overwrite)
                 throw MakeStringException(-1,"Destination file %s already exists",lfn);
-            if (dfile->querySuperFile())
-                dfile->detach();
-            else {
+            if (!dfile->querySuperFile())
+            {
                 if (ftree->hasProp("Attr/@fileCrc")&&ftree->getPropInt64("Attr/@size")&&
                     ((unsigned)ftree->getPropInt64("Attr/@fileCrc")==(unsigned)dfile->queryAttributes().getPropInt64("@fileCrc"))&&
-                    (ftree->getPropInt64("Attr/@size")==dfile->getFileSize(false,false))) {
+                    (ftree->getPropInt64("Attr/@size")==dfile->getFileSize(false,false)))
+                {
                     PROGLOG("File copy of %s not done as file unchanged",srclfn);
                     return;
                 }
-                dfile->detach();
-                dfile->removePhysicalPartFiles(NULL,NULL);
             }
+            dfile->detach();
             dfile.clear();
         }
-        if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_File))==0) {
+        if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_File))==0)
+        {
             assertex(copier);
             if (!copier->copyFile(lfn,daliep,srclfn,srcuser,user))
                 throw MakeStringException(-1,"File %s could not be copied",lfn);
 
         }
-        else if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_SuperFile))==0) {
+        else if (strcmp(ftree->queryName(),queryDfsXmlBranchName(DXB_SuperFile))==0)
+        {
             StringArray subfiles;
             Owned<IPropertyTreeIterator> piter = ftree->getElements("SubFile");
-            ForEach(*piter) {
+            ForEach(*piter)
+            {
                 const char *name = piter->query().queryProp("@name");
                 CDfsLogicalFileName dlfn;
                 dlfn.set(name);
@@ -932,12 +929,11 @@ public:
             Owned<IDistributedSuperFile> sfile = queryDistributedFileDirectory().createSuperFile(lfn,user,true,false);
             if (!sfile)
                 throw MakeStringException(-1,"SuperFile %s could not be created",lfn);
-            ForEachItemIn(i,subfiles) {
+            ForEachItemIn(i,subfiles)
                 sfile->addSubFile(subfiles.item(i));
-            }
-
         }
-        else {
+        else
+        {
             StringBuffer s;
             throw MakeStringException(-1,"Source file %s in Dali %s is not a file or superfile",srclfn,daliep.getUrlStr(s).str());
         }

dali/dfu/dfuwu.cpp

@@ -1678,11 +1678,6 @@ class CDFUoptions: public CLinkedDFUWUchild, implements IDFUoptions
 public:
     IMPLEMENT_DFUWUCHILD;
 
-    bool getNoDelete() const
-    {
-        return queryRoot()->getPropInt("@nodelete")!=0;
-    }
-
     bool getNoSplit() const
     {
         return queryRoot()->getPropInt("@nosplit")!=0;

dali/dfu/dfuwu.hpp

@@ -136,7 +136,6 @@ enum DFUclusterPartDiskMapping // legacy - should use ClusterPartDiskMapSpec ins
 
 interface IConstDFUoptions : extends IInterface
 {
-    virtual bool getNoDelete() const = 0;
     virtual bool getNoSplit() const = 0;
     virtual bool getReplicate() const = 0;
     virtual bool getRecover() const = 0;

dali/dfuplus/dfuplus.cpp

@@ -886,14 +886,6 @@ int CDfuPlusHelper::rundafs()
 
 int CDfuPlusHelper::remove()
 {
-    bool nodelete = false;
-    const char* ndstr = NULL;
-    if((ndstr = globals->queryProp("nodelete")) != NULL)
-    {
-        if(strcmp(ndstr, "1") == 0)
-        nodelete = true;
-    }
-
     StringArray files;
     const char* name = globals->queryProp("name");
     const char* names = globals->queryProp("names");
@@ -960,7 +952,6 @@ int CDfuPlusHelper::remove()
     
     Owned<IClientDFUArrayActionRequest> req = dfuclient->createDFUArrayActionRequest();
     req->setType("Delete");
-    req->setNoDelete(nodelete);
     req->setLogicalFiles(files);
 
     Owned<IClientDFUArrayActionResponse> resp = dfuclient->DFUArrayAction(req);

dali/dfuplus/main.cpp

@@ -106,7 +106,6 @@ void handleSyntax()
     out.append("        name=<logical-name>\n");
     out.append("        names=<multiple-logical-names-separated-by-comma>\n");
     out.append("        namelist=<logical-name-list-in-file>\n");
-    out.append("        nodelete=0|1    -- optional\n");
     out.append("    rename options:\n");
     out.append("        srcname=<source-logical-name>\n");
     out.append("        dstname=<destination-logical-name>\n");

dali/ft/daft.cpp

@@ -106,7 +106,7 @@ void CDistributedFileSystem::move(IDistributedFile * from, IDistributedFile * to
     sprayer->setTarget(to);
     sprayer->spray();
     //sprayer->removeSource();
-    remove(from);
+    from->detach();
 }
 
 void CDistributedFileSystem::replicate(IDistributedFile * from, IGroup *destgroup, IPropertyTree * recovery, IRemoteConnection * recoveryConnection, IDFPartFilter *filter, IPropertyTree * options, IDaftProgress * progress, IAbortRequestCallback * abort, const char *wuid)
@@ -212,17 +212,6 @@ offset_t CDistributedFileSystem::getSize(IDistributedFile * file, bool forceget,
     return totalSize;
 }
 
-// FIXME: This should NOT call detach / removePhysical directly!
-bool CDistributedFileSystem::remove(IDistributedFile * file,const char *cluster,IMultiException *mexcept, unsigned timeoutms)
-{
-    // this is now equivalent to removePhysicalPartFiles as linux uses dafilesrv by default
-    if (!cluster||((file->findCluster(cluster)==0)&&(file->numClusters()==1))) {
-        cluster = NULL; // deleting the last cluster removes the file
-        file->detach(timeoutms);
-    }
-    return file->removePhysicalPartFiles(cluster,mexcept); // this is bit cavalier with errors - but better orphans than inconsistant DFS
-}
-
 bool CDistributedFileSystem::compress(IDistributedFile * file)
 {
     bool ok = true;

dali/ft/daft.hpp

@@ -72,7 +72,6 @@ interface IDistributedFileSystem : public IInterface
     virtual offset_t getSize(IDistributedFile * file,
                              bool forceget=false,                               // if true gets physical size (ignores cached attribute)
                              bool dontsetattr=true) = 0;                        // if true doesn't set attribute when physical size got
-    virtual bool remove(IDistributedFile * file,const char *clustername=NULL,IMultiException *mexcept=NULL, unsigned timeoutms=INFINITE) = 0;
     virtual bool compress(IDistributedFile * file) = 0;                                                  
     virtual offset_t getCompressedSize(IDistributedFile * part) = 0;
 

dali/ft/daft.ipp

@@ -42,7 +42,6 @@ public:
 
 //operations on a single file.
     virtual offset_t getSize(IDistributedFile * file,bool forceget,bool dontsetattr);
-    virtual bool remove(IDistributedFile * file,const char *cluster=NULL,IMultiException *mexcept=NULL, unsigned timeoutms=INFINITE);
     virtual bool compress(IDistributedFile * file);                                                  
     virtual offset_t getCompressedSize(IDistributedFile * part);
 

docs/HPCCClientTools/CT_Mods/CT_Comm_Line_DFU.xml

@@ -750,16 +750,6 @@ dfuplus action=dspray srcname=le::imagedb
 
                     <entry>The logical name of the file to remove.</entry>
                   </row>
-
-                  <row>
-                    <entry><emphasis><emphasis><emphasis>nodelete</emphasis>
-                    </emphasis></emphasis></entry>
-
-                    <entry>A boolean flag (0 | 1) indicating whether to
-                    physically delete the file in addition to removing its
-                    listing from the DFU. If omitted, the default is
-                    0—physically delete the file.</entry>
-                  </row>
                 </tbody>
               </tgroup>
             </informaltable> Example:</para>

esp/services/ws_dfu/ws_dfuService.cpp

@@ -1150,144 +1150,73 @@ bool CWsDfuEx::DFUDeleteFiles(IEspContext &context, IEspDFUArrayActionRequest &r
     StringArray superFileNames, filesCannotBeDeleted;
     for(int j = 0; j < 2; j++) //j=0: delete superfiles first
     {
-        for(unsigned i = 0; i < req.getLogicalFiles().length();i++)
+        for(unsigned i = 0; i < req.getLogicalFiles().length(); i++)
         {
-            const char* file = req.getLogicalFiles().item(i);
-            if(!file || !*file)
+            const char* filename = req.getLogicalFiles().item(i);
+            if(!filename || !*filename)
                 continue;
 
-            unsigned len = strlen(file);
-            const char* cluster = NULL;
-            const char *pCh = strchr(file, '@');
-            if (pCh)
-            {
-                len = pCh - file;
-                if (len+1 < strlen(file))
-                    cluster = pCh + 1;
-            }
-
-            StringBuffer logicalFileName;
-            char* curfile = new char[len+1];
-            strncpy(curfile, file, len);
-            curfile[len] = 0;
-            logicalFileName.append(curfile);
-            delete [] curfile;
-
             if (j>0)
-            { //now, we want to skip superfiles and the files which cannot do the lookup.
-                bool superFile = false;
-                ForEachItemIn(ii, superFileNames)
-                {
-                    const char* file = superFileNames.item(ii);
-                    if (file && streq(file, logicalFileName.str()))
-                    {
-                        superFile = true;
-                        break;
-                    }
-                }
+            { // 2nd pass, now we want to skip superfiles and the files which cannot do the lookup.
 
-                if (superFile)
+                if (superFileNames.contains(filename))
                     continue;
 
-                bool fileCannotBeDeleted = false;
-                ForEachItemIn(i, filesCannotBeDeleted)
-                {
-                    const char* file = filesCannotBeDeleted.item(i);
-                    if (file && streq(file, logicalFileName.str()))
-                    {
-                        fileCannotBeDeleted = true;
-                        break;
-                    }
-                }
-
-                if (fileCannotBeDeleted)
+                if (filesCannotBeDeleted.contains(filename))
                     continue;
             }
 
+            Owned<IDistributedFile> df;
             try
             {
-                Owned<IDistributedFile> df = queryDistributedFileDirectory().lookup(logicalFileName.str(), userdesc, true) ;
+                df.setown(queryDistributedFileDirectory().lookup(filename, userdesc, true));
                 if(!df)
                 {
-                    returnStr.appendf("<Message><Value>Cannot delete %s: file not found</Value></Message>", logicalFileName.str());
-                    filesCannotBeDeleted.append(logicalFileName);
+                    returnStr.appendf("<Message><Value>Cannot delete %s: file not found</Value></Message>", filename);
+                    filesCannotBeDeleted.append(filename);
                     continue;
                 }
-
-                if (j<1) //j=0: delete superfiles first
-                {
-                    if(!df->querySuperFile())
-                        continue;
-
-                    superFileNames.append(logicalFileName);
-                }
-
-                DBGLOG("CWsDfuEx::DFUDeleteFiles User=%s Action=Delete File=%s",username.str(), logicalFileName.str());
-
-                CDfsLogicalFileName lfn;
-                StringBuffer cname(cluster);
-                lfn.set(logicalFileName.str());
-                if (!cname.length())
-                    lfn.getCluster(cname);  // see if cluster part of LogicalFileName
-
-                bool deleted;
-                if(!req.getNoDelete() && !df->querySuperFile())
-                {
-                    Owned<IMultiException> pExceptionHandler = MakeMultiException();
-                    deleted = queryDistributedFileSystem().remove(df,cname.length()?cname.str():NULL,pExceptionHandler, REMOVE_FILE_SDS_CONNECT_TIMEOUT);
-                    StringBuffer errorStr;
-                    pExceptionHandler->errorMessage(errorStr);
-                    if (errorStr.length() > 0)
-                    {
-                        returnStr.appendf("<Message><Value>%s</Value></Message>",errorStr.str());
-                        DBGLOG("%s", errorStr.str());
-                    }
-                    else if (!deleted)
-                    {
-                        returnStr.appendf("<Message><Value>Logical File %s not deleted. No error message.</Value></Message>",logicalFileName.str());
-                        DBGLOG("Logical File %s not deleted. No error message.\n",logicalFileName.str());
-                    }
-                    else
-                    {
-                        PrintLog("Deleted Logical File: %s\n",logicalFileName.str());
-                        returnStr.appendf("<Message><Value>Deleted File %s</Value></Message>",logicalFileName.str());
-                    }
-                }
-                else
-                {
-                    df.clear(); 
-                    deleted = queryDistributedFileDirectory().removeEntry(logicalFileName.str(), userdesc, NULL, REMOVE_FILE_SDS_CONNECT_TIMEOUT); // this can remove clusters also
-                    if (deleted)
-                    {
-                        PrintLog("Detached File: %s\n",logicalFileName.str());
-                        returnStr.appendf("<Message><Value>Detached File %s</Value></Message>", logicalFileName.str());
-                    }
-                    else
-                    {
-                        returnStr.appendf("<Message><Value>File %s not detached.</Value></Message>",logicalFileName.str());
-                        DBGLOG("File %s not detached.\n",logicalFileName.str());
-                    }
-                }
-
-                if (!deleted)
-                    return false;
             }
             catch(IException* e)
             {
-                filesCannotBeDeleted.append(logicalFileName);
+                filesCannotBeDeleted.append(filename);
 
                 StringBuffer emsg;
                 e->errorMessage(emsg);
                 if((e->errorCode() == DFSERR_CreateAccessDenied) && (req.getType() != NULL))
-                {
-                    emsg.replaceString("Create ", "Delete ");               
-                }
+                    emsg.replaceString("Create ", "Delete ");
 
-                returnStr.appendf("<Message><Value>Cannot delete %s: %s</Value></Message>", logicalFileName.str(), emsg.str());
+                returnStr.appendf("<Message><Value>Cannot delete %s: %s</Value></Message>", filename, emsg.str());
             }
             catch(...)
             {
-                returnStr.appendf("<Message><Value>Cannot delete %s: unknown exception.</Value></Message>", logicalFileName.str());
+                returnStr.appendf("<Message><Value>Cannot delete %s: unknown exception.</Value></Message>", filename);
+            }
+            if (df)
+            {
+                if (0==j) // skip non-super files on 1st pass
+                {
+                    if(!df->querySuperFile())
+                        continue;
+
+                    superFileNames.append(filename);
+                }
+
+                DBGLOG("CWsDfuEx::DFUDeleteFiles User=%s Action=Delete File=%s",username.str(), filename);
+                try
+                {
+                    df->detach(REMOVE_FILE_SDS_CONNECT_TIMEOUT);
+                    PROGLOG("Deleted Logical File: %s\n",filename);
+                    returnStr.appendf("<Message><Value>Deleted File %s</Value></Message>", filename);
+                }
+                catch (IException *e)
+                {
+                    StringBuffer errorMsg;
+                    e->errorMessage(errorMsg);
+                    PROGLOG("%s", errorMsg.str());
+                    e->Release();
+                    returnStr.appendf("<Message><Value>%s</Value></Message>", errorMsg.str());
+                }
             }
         }
     }
@@ -1376,7 +1305,7 @@ bool CWsDfuEx::onDFUArrayAction(IEspContext &context, IEspDFUArrayActionRequest
             DBGLOG("CWsDfuEx::onDFUArrayAction User=%s Action=%s File=%s",username.str(),req.getType(), file);
             try
             {
-                onDFUAction(userdesc.get(), curfile, cluster, req.getType(), req.getNoDelete(), returnStr);
+                onDFUAction(userdesc.get(), curfile, cluster, req.getType(), returnStr);
             }
             catch(IException* e)
             {
@@ -1421,13 +1350,13 @@ bool CWsDfuEx::onDFUArrayAction(IEspContext &context, IEspDFUArrayActionRequest
     return true;
 }
 
-bool CWsDfuEx::onDFUAction(IUserDescriptor* udesc, const char* LogicalFileName, const char* ClusterName,const char* ActionType,bool nodelete, StringBuffer& returnStr)
+bool CWsDfuEx::onDFUAction(IUserDescriptor* udesc, const char* LogicalFileName, const char* ClusterName, const char* ActionType, StringBuffer& returnStr)
 {
     //No 'try/catch' is needed for this method since it will be called internally.
     if (strcmp(Action_Delete ,ActionType) == 0)
     {
         LogicFileWrapper Logicfile;
-        if (!Logicfile.doDeleteFile(LogicalFileName,ClusterName,nodelete, returnStr, udesc))
+        if (!Logicfile.doDeleteFile(LogicalFileName,ClusterName, returnStr, udesc))
             return false;
     }
     else if (strcmp(Action_AddtoSuperfile ,ActionType) == 0)

esp/services/ws_dfu/ws_dfuService.hpp

@@ -111,7 +111,7 @@ private:
     bool checkDescription(const char *description, const char *descriptionFilter);
     void getDefFile(IUserDescriptor* udesc, const char* FileName,StringBuffer& returnStr);
     void xsltTransformer(const char* xsltPath,StringBuffer& source,StringBuffer& returnStr);
-    bool onDFUAction(IUserDescriptor* udesc, const char* LogicalFileName,const char* ClusterName,const char* ActionType, bool nodelete, StringBuffer& returnStr);
+    bool onDFUAction(IUserDescriptor* udesc, const char* LogicalFileName, const char* ClusterName, const char* ActionType, StringBuffer& returnStr);
     bool checkFileContent(IEspContext &context, IUserDescriptor* udesc, const char * logicalName, const char * cluster);
     void getRoxieClusterConfig(char const * clusterType, char const * clusterName, char const * processName, StringBuffer& netAddress, int& port);
     //bool getRoxieQueriesForFile(const char* logicalName, const char* cluster, StringArray& roxieQueries);

esp/smc/SMCLib/LogicFileWrapper.cpp

@@ -55,49 +55,39 @@ void LogicFileWrapper::FindClusterName(const char* logicalName, StringBuffer& re
     }
 }
 
-bool LogicFileWrapper::doDeleteFile(const char* LogicalFileName,const char *cluster, bool nodelete,StringBuffer& returnStr, IUserDescriptor* udesc)
+bool LogicFileWrapper::doDeleteFile(const char* logicalName,const char *cluster, StringBuffer& returnStr, IUserDescriptor* udesc)
 {
     CDfsLogicalFileName lfn;
-    StringBuffer cname(cluster);
-    lfn.set(LogicalFileName);
-    if (!cname.length())
-        lfn.getCluster(cname);  // see if cluster part of LogicalFileName
+    lfn.set(logicalName);
+    StringBuffer cname;
+    lfn.getCluster(cname);
+    if (0 == cname.length()) // if has no cluster, use supplied cluster
+        lfn.setCluster(cluster);
+    lfn.get(cname.clear(), false, true); // get file@cluster form;
 
-    Owned<IDistributedFile> df = queryDistributedFileDirectory().lookup(LogicalFileName, udesc, true) ;
-    if(!df)
-    {
-        returnStr.appendf("<Message><Value>File %s not found</Value></Message>", LogicalFileName);
-        return false;
-    }
-
-    bool deleted;
-    if(!nodelete && !df->querySuperFile())
+    try
     {
-        Owned<IMultiException> pExceptionHandler = MakeMultiException();
-        deleted = queryDistributedFileSystem().remove(df,cname.length()?cname.str():NULL,pExceptionHandler);
-        StringBuffer errorStr;
-        pExceptionHandler->errorMessage(errorStr);
-        if (errorStr.length() > 0)
+        Owned<IDistributedFile> df = queryDistributedFileDirectory().lookup(cname.str(), udesc, true) ;
+        if(!df)
         {
-            returnStr.appendf("<Message><Value>%s</Value></Message>",errorStr.str());
-            DBGLOG("%s", errorStr.str());
-        }
-        else {
-            PrintLog("Deleted Logical File: %s\n",LogicalFileName);
-            returnStr.appendf("<Message><Value>Deleted File %s</Value></Message>",LogicalFileName);
+            returnStr.appendf("<Message><Value>File %s not found</Value></Message>", cname.str());
+            return false;
         }
 
+        df->detach();
+        returnStr.appendf("<Message><Value>Deleted File %s</Value></Message>", cname.str());
+        DBGLOG("%s", returnStr.str());
+        return true;
     }
-    else
+    catch (IException *e)
     {
-        df.clear(); 
-        deleted = queryDistributedFileDirectory().removeEntry(LogicalFileName,udesc); // this can remove clusters also
-        if (deleted)
-            returnStr.appendf("<Message><Value>Detached File %s</Value></Message>", LogicalFileName);
+        StringBuffer errorMsg;
+        e->errorMessage(returnStr);
+        e->Release();
+        PROGLOG("%s", errorMsg.str());
+        returnStr.appendf("<Message><Value>Failed to delete File %s, error: %s</Value></Message>", cname.str(), errorMsg.str());
     }
-
-
-    return deleted; 
+    return false;
 }
 
 bool LogicFileWrapper::doCompressFile(const char* name,StringBuffer& returnStr, IUserDescriptor* udesc)

esp/smc/SMCLib/LogicFileWrapper.hpp

@@ -47,7 +47,7 @@ public:
     IMPLEMENT_IINTERFACE;
     LogicFileWrapper();
     virtual ~LogicFileWrapper();
-    bool doDeleteFile(const char* name, const char *cluster, bool nodelete, StringBuffer& returnStr, IUserDescriptor* udesc = NULL);
+    bool doDeleteFile(const char* name, const char *cluster, StringBuffer& returnStr, IUserDescriptor* udesc = NULL);
     bool doCompressFile(const char* name,StringBuffer& returnStr, IUserDescriptor* udesc = 0);
     void FindClusterName(const char* logicalName,StringBuffer& returnCluster, IUserDescriptor* udesc = 0);
 

thorlcr/mfilemanager/thmfilemanager.cpp

@@ -162,11 +162,7 @@ class CFileManager : public CSimpleInterface, implements IThorFileManager
                 throw MakeThorException(0, "Failed to remove external file: %s", lfn.str());
         }
         else
-        {
             file.detach();
-            if (!file.querySuperFile())
-                file.removePhysicalPartFiles(); 
-        }
     }
 
 public: