HPCC-9397 Roxie remote file resolution issues

Cache all group lookups, not just the most recent.

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

dali/base/dadfs.cpp

@@ -6069,13 +6069,25 @@ public:
 
 #define GROUP_CACHE_INTERVAL (1000*60)
 
+class CNamedGroupCacheEntry: public CInterface
+{
+public:
+    Linked<IGroup> group;
+    StringAttr name;
+    StringAttr groupdir;
+    unsigned cachedtime;
+
+    CNamedGroupCacheEntry(IGroup *_group, const char *_name, const char *_dir)
+    : group(_group), name(_name), groupdir(_dir)
+    {
+        cachedtime = msTick();
+    }
+};
+
 class CNamedGroupStore: public CInterface, implements INamedGroupStore
 {
     CriticalSection cachesect;
-    Owned<IGroup> cachedgroup;
-    StringAttr cachedname;
-    StringAttr cachedgroupdir;
-    unsigned cachedtime;
+    CIArrayOf<CNamedGroupCacheEntry> cache;
     unsigned defaultTimeout;
 
 public:
@@ -6084,7 +6096,6 @@ public:
     CNamedGroupStore()
     {
         defaultTimeout = INFINITE;
-        cachedtime = 0;
     }
 
     IGroup *dolookup(const char *logicalgroupname,IRemoteConnection *conn, StringBuffer *dirret)
@@ -6138,24 +6149,30 @@ public:
             logicalgroupname = gname.str();
         }
         StringAttr groupdir;
+        bool cached = false;
+        unsigned timeNow = msTick();
         {
             CriticalBlock block(cachesect);
-            if (cachedgroup.get()) {
-                if (msTick()-cachedtime>GROUP_CACHE_INTERVAL) {
-                    cachedgroup.clear();
-                    cachedname.clear();
-                    cachedgroupdir.clear();
+            ForEachItemInRev(idx, cache)
+            {
+                CNamedGroupCacheEntry &entry = cache.item(idx);
+                if (timeNow-entry.cachedtime > GROUP_CACHE_INTERVAL)
+                {
+                    cache.remove(idx);
                 }
-                else if (strcmp(gname.str(),cachedname.get())==0) {
-                    cachedtime = msTick();
-                    if (range.length()==0) {
+                else if (strcmp(gname.str(),entry.name.get())==0)
+                {
+                    cached = true;
+                    if (range.length()==0)
+                    {
                         if (dirret)
-                            dirret->append(cachedgroupdir);
-                        return cachedgroup.getLink();
+                            dirret->append(entry.groupdir);
+                        return entry.group.getLink();
                     }
                     // there is a range so copy to epa
-                    cachedgroup->getSocketEndpoints(epa);
-                    groupdir.set(cachedgroupdir);
+                    entry.group->getSocketEndpoints(epa);
+                    groupdir.set(entry.groupdir);
+                    break;
                 }
             }
         }
@@ -6168,7 +6185,7 @@ public:
                 s+=2;
                 if (*s) {
                     Owned<INode> dali = createINode(eps.str());
-                    if (!dali || !getRemoteGroup(epa, dali, s, FOREIGN_DALI_TIMEOUT, groupdir))
+                    if (!dali || !getRemoteGroup(dali, s, FOREIGN_DALI_TIMEOUT, groupdir, epa))
                         return NULL;
                 }
             }
@@ -6196,30 +6213,33 @@ public:
                 epa.append(ep);
             }
         }
-        IGroup *ret = createIGroup(epa);
+        Owned<IGroup> ret = createIGroup(epa);
+        if (!cached)
         {
             CriticalBlock block(cachesect);
-            cachedgroup.set(ret);
-            cachedname.set(gname);
-            cachedgroupdir.set(groupdir);
-            cachedtime = msTick();
+            cache.append(*new CNamedGroupCacheEntry(ret, gname, groupdir));
         }
-        if (range.length()) {
+        if (range.length())
+        {
             SocketEndpointArray epar;
             const char *s = range.str();
-            while (*s) {
+            while (*s)
+            {
                 unsigned start = 0;
-                while (isdigit(*s)) {
+                while (isdigit(*s))
+                {
                     start = start*10+*s-'0';
                     s++;
                 }
                 if (!start)
                     break;
                 unsigned end;
-                if (*s=='-') {
+                if (*s=='-')
+                {
                     s++;
                     end = 0;
-                    while (isdigit(*s)) {
+                    while (isdigit(*s))
+                    {
                         end = end*10+*s-'0';
                         s++;
                     }
@@ -6228,7 +6248,8 @@ public:
                 }
                 else 
                     end = start;
-                if ((start>epa.ordinality())||(end>epa.ordinality())) {
+                if ((start>epa.ordinality())||(end>epa.ordinality()))
+                {
                     s = range.str();
                     break;
                 }
@@ -6244,12 +6265,11 @@ public:
             }
             if (*s) 
                 throw MakeStringException(-1,"Invalid group range %s",range.str());
-            ::Release(ret);
-            ret = createIGroup(epar);
+            ret.setown(createIGroup(epar));
         }
         if (dirret)
             dirret->append(groupdir);
-        return ret;
+        return ret.getClear();
     }
 
     IGroup *lookup(const char *logicalgroupname)
@@ -6337,11 +6357,10 @@ public:
         connlock.conn->queryRoot()->removeProp(prop.str()); 
         doadd(connlock,name.str(),group,cluster,dir);
         {                                                           
-            CriticalBlock block(cachesect);                     
-            cachedgroup.set(group); // may be NULL
-            cachedname.set(name.str());
-            cachedgroupdir.set(dir);
-            cachedtime = msTick();
+            CriticalBlock block(cachesect);
+            cache.kill();
+            if (group)
+                cache.append(*new CNamedGroupCacheEntry(group, name.str(), dir));
         }
     }
 
@@ -6408,9 +6427,7 @@ public:
             }
         }
         CriticalBlock block(cachesect);
-        cachedgroup.clear();
-        cachedname.clear();
-        cachedgroupdir.clear();
+        cache.kill();
     }
 
     unsigned setDefaultTimeout(unsigned timems)
@@ -6421,7 +6438,7 @@ public:
     }
 
 private:
-    bool getRemoteGroup(SocketEndpointArray &epa, const INode *foreigndali, const char *gname, unsigned foreigndalitimeout, StringAttr &groupdir)
+    bool getRemoteGroup(const INode *foreigndali, const char *gname, unsigned foreigndalitimeout, StringAttr &groupdir, SocketEndpointArray &epa)
     {
         StringBuffer lcname(gname);
         gname = lcname.trim().toLowerCase().str();

dali/dfu/dfuutil.cpp

@@ -404,7 +404,7 @@ class CFileCloner
 
     void cloneSubFile(IPropertyTree *ftree,const char *destfilename, INode *srcdali)   // name already has prefix added
     {
-        Owned<IFileDescriptor> srcfdesc = deserializeFileDescriptorTree(ftree,&queryNamedGroupStore(),0);
+        Owned<IFileDescriptor> srcfdesc = deserializeFileDescriptorTree(ftree, NULL, 0);
         const char * kind = srcfdesc->queryProperties().queryProp("@kind");
         bool iskey = kind&&(strcmp(kind,"key")==0);
 

roxie/ccd/ccddali.cpp

@@ -276,9 +276,6 @@ private:
 
     IFileDescriptor *recreateCloneSource(IFileDescriptor *srcfdesc, const char *destfilename)
     {
-        const char * kind = srcfdesc->queryProperties().queryProp("@kind");
-        bool iskey = kind&&(strcmp(kind,"key")==0);
-
         Owned<IFileDescriptor> dstfdesc = createFileDescriptor(srcfdesc->getProperties());
         // calculate dest dir
 
@@ -396,7 +393,7 @@ public:
 
     IFileDescriptor *checkClonedFromRemote(const char *_lfn, IFileDescriptor *fdesc, bool cacheIt)
     {
-        // MORE - may want to cache to avoid a lookup per channel
+        // NOTE - we rely on the fact that  queryNamedGroupStore().lookup caches results,to avoid excessive load on remote dali
         if (_lfn && !strnicmp(_lfn, "foreign", 7)) //if need to support dali hopping should add each remote location
             return NULL;
         if (!fdesc || !fdesc->queryProperties().hasProp("@cloneFrom"))

roxie/ccd/ccdfile.cpp

@@ -1046,7 +1046,7 @@ public:
                                      const StringArray &deployedLocationInfo, bool startFileCopy)
     {
         IPropertyTree &partProps = pdesc->queryProperties();
-        offset_t dfsSize = partProps.getPropInt64("@size");
+        offset_t dfsSize = partProps.getPropInt64("@size", -1);
         unsigned crc;
         if (!pdesc->getCrc(crc))
             crc = 0;
@@ -1076,7 +1076,7 @@ public:
             Linked<ILazyFileIO> f = files.getValue(localLocation);
             if (f && f->isAlive())
             {
-                if ((dfsSize != -1 && dfsSize != f->getSize()) ||
+                if ((dfsSize != (offset_t) -1 && dfsSize != f->getSize()) ||
                     (!dfsDate.isNull() && !dfsDate.equals(*f->queryDateTime(), false)))
                 {
                     StringBuffer modifiedDt;