From f384cc671f6b6445ac380a8f9f6081f1d4add773 Mon Sep 17 00:00:00 2001 From: Gavin Halliday Date: Fri, 13 Dec 2024 17:12:18 +0000 Subject: [PATCH] Resolve the conflict between thor and roxie semantics Signed-off-by: Gavin Halliday --- dali/base/dafdesc.cpp | 6 +++--- dali/base/dafdesc.hpp | 2 +- dali/daliadmin/daadmin.cpp | 2 +- dali/dfu/dfuserver.cpp | 2 +- dali/dfu/dfuutil.cpp | 2 +- dali/dfuxref/dfuxrefmain.cpp | 2 +- dali/sasha/saserver.cpp | 2 +- ecl/eclagent/eclagent.cpp | 2 +- esp/platform/espcfg.cpp | 2 +- esp/platform/espp.cpp | 2 +- roxie/ccd/ccddali.cpp | 2 +- roxie/ccd/ccdmain.cpp | 2 +- system/jlib/jptree.cpp | 21 ++++++++------------- system/jlib/jptree.hpp | 2 +- thorlcr/master/thmastermain.cpp | 4 +--- 15 files changed, 24 insertions(+), 31 deletions(-) diff --git a/dali/base/dafdesc.cpp b/dali/base/dafdesc.cpp index 8940ef61093..a03866bcce3 100644 --- a/dali/base/dafdesc.cpp +++ b/dali/base/dafdesc.cpp @@ -3794,16 +3794,16 @@ static void doInitializeStorageGroups(bool createPlanesFromGroups, IPropertyTree setupContainerizedStorageLocations(); } -void initializeStorageGroups(bool createPlanesFromGroups) +void initializeStoragePlanes(bool createPlanesFromGroups, bool threadSafe) { auto updateFunc = [createPlanesFromGroups](IPropertyTree * newComponentConfiguration, IPropertyTree * newGlobalConfiguration) { //MORE: createPlanesFromGroup will never be updated.... - PROGLOG("initializeStorageGroups update"); + PROGLOG("initializeStoragePlanes update"); doInitializeStorageGroups(createPlanesFromGroups, newGlobalConfiguration); }; - configUpdateHook.installOnce(updateFunc); + configUpdateHook.installModifierOnce(updateFunc, threadSafe); } bool getDefaultStoragePlane(StringBuffer &ret) diff --git a/dali/base/dafdesc.hpp b/dali/base/dafdesc.hpp index 377bfd1ab67..41640c910f4 100644 --- a/dali/base/dafdesc.hpp +++ b/dali/base/dafdesc.hpp @@ -406,7 +406,7 @@ extern da_decl StringBuffer &getPartMask(StringBuffer &ret,const char *lname=NUL extern da_decl void setPartMask(const char * mask); extern da_decl bool setReplicateDir(const char *name,StringBuffer &out, bool isrep=true,const char *baseDir=NULL,const char *repDir=NULL); // changes directory of name passed to backup directory -extern da_decl void initializeStorageGroups(bool createPlanesFromGroups); +extern da_decl void initializeStoragePlanes(bool createPlanesFromGroups, bool threadSafe); // threadSafe should be true if no other threads will be accessing the global config extern da_decl bool getDefaultStoragePlane(StringBuffer &ret); extern da_decl bool getDefaultSpillPlane(StringBuffer &ret); extern da_decl bool getDefaultIndexBuildStoragePlane(StringBuffer &ret); diff --git a/dali/daliadmin/daadmin.cpp b/dali/daliadmin/daadmin.cpp index 4274d44db10..87988ead887 100644 --- a/dali/daliadmin/daadmin.cpp +++ b/dali/daliadmin/daadmin.cpp @@ -507,7 +507,7 @@ bool dfspart(const char *lname, IUserDescriptor *userDesc, unsigned partnum, Str void dfsmeta(const char *filename,IUserDescriptor *userDesc, bool includeStorage) { //This function isn't going to work on a container system because it won't have access to the storage planes - initializeStorageGroups(true); + initializeStoragePlanes(true, true); ResolveOptions options = ROpartinfo|ROdiskinfo|ROsizes; if (includeStorage) options = options | ROincludeLocation; diff --git a/dali/dfu/dfuserver.cpp b/dali/dfu/dfuserver.cpp index 514bcd3655a..47417d272a3 100644 --- a/dali/dfu/dfuserver.cpp +++ b/dali/dfu/dfuserver.cpp @@ -198,7 +198,7 @@ int main(int argc, const char *argv[]) IPropertyTree * config = nullptr; installDefaultFileHooks(config); - initializeStorageGroups(true); + initializeStoragePlanes(true, true); } StringBuffer queue, monitorQueue; #ifndef _CONTAINERIZED diff --git a/dali/dfu/dfuutil.cpp b/dali/dfu/dfuutil.cpp index ead2a9478c8..91cb09981bc 100644 --- a/dali/dfu/dfuutil.cpp +++ b/dali/dfu/dfuutil.cpp @@ -513,7 +513,7 @@ class CFileCloner dstfdesc->setDefaultDir(dstdir.str()); Owned plane = getDataStoragePlane(cluster1, false); - if (plane) // I think it should always exist, even in bare-metal.., but guard against it not for now (assumes initializeStorageGroups has been called) + if (plane) // I think it should always exist, even in bare-metal.., but guard against it not for now (assumes initializeStoragePlanes has been called) { DBGLOG("cloneSubFile: destfilename='%s', plane='%s', dirPerPart=%s", destfilename, cluster1.get(), boolToStr(plane->queryDirPerPart())); diff --git a/dali/dfuxref/dfuxrefmain.cpp b/dali/dfuxref/dfuxrefmain.cpp index f9d772a69ea..650d3184679 100644 --- a/dali/dfuxref/dfuxrefmain.cpp +++ b/dali/dfuxref/dfuxrefmain.cpp @@ -76,7 +76,7 @@ int main(int argc, char* argv[]) try { initClientProcess(group, DCR_XRef); - initializeStorageGroups(true); + initializeStoragePlanes(true, true); StringArray args, clusters; bool backupcheck = false; unsigned mode = PMtextoutput|PMcsvoutput|PMtreeoutput; diff --git a/dali/sasha/saserver.cpp b/dali/sasha/saserver.cpp index 16f808542cd..acdf1775772 100644 --- a/dali/sasha/saserver.cpp +++ b/dali/sasha/saserver.cpp @@ -409,7 +409,7 @@ int main(int argc, const char* argv[]) else { addAbortHandler(actionOnAbort); - initializeStorageGroups(true); + initializeStoragePlanes(true, true); #ifdef _CONTAINERIZED service = serverConfig->queryProp("@service"); if (isEmptyString(service)) diff --git a/ecl/eclagent/eclagent.cpp b/ecl/eclagent/eclagent.cpp index 0af88caa6e2..0acb00ee2ce 100644 --- a/ecl/eclagent/eclagent.cpp +++ b/ecl/eclagent/eclagent.cpp @@ -3785,7 +3785,7 @@ extern int HTHOR_API eclagent_main(int argc, const char *argv[], OwnedgetPropInt("@dafilesrvConnectTimeout", 10)*1000; const unsigned dafilesrvReadTimeout = m_cfg->getPropInt("@dafilesrvReadTimeout", 10)*1000; diff --git a/esp/platform/espp.cpp b/esp/platform/espp.cpp index 20d3898d1f3..230033abe45 100644 --- a/esp/platform/espp.cpp +++ b/esp/platform/espp.cpp @@ -578,7 +578,7 @@ int init_main(int argc, const char* argv[]) config->checkESPCache(*server.get()); initializeMetrics(config); - initializeStorageGroups(daliClientActive()); + initializeStoragePlanes(daliClientActive(), true); } catch(IException* e) { diff --git a/roxie/ccd/ccddali.cpp b/roxie/ccd/ccddali.cpp index bdab2a2c650..4d9da90891e 100644 --- a/roxie/ccd/ccddali.cpp +++ b/roxie/ccd/ccddali.cpp @@ -805,7 +805,7 @@ class CRoxieDaliHelper : implements IRoxieDaliHelper, public CInterface waitToConnect -= delay; } } - initializeStorageGroups(daliHelper->connected()); + initializeStoragePlanes(daliHelper->connected(), false); // This can be called while queries are running - so is not thread safe return daliHelper; } diff --git a/roxie/ccd/ccdmain.cpp b/roxie/ccd/ccdmain.cpp index fa36b53862e..b466d8e2d7d 100644 --- a/roxie/ccd/ccdmain.cpp +++ b/roxie/ccd/ccdmain.cpp @@ -1463,7 +1463,7 @@ int CCD_API roxie_main(int argc, const char *argv[], const char * defaultYaml) #endif //MORE: I'm not sure where this should go, or how it fits in. Possibly the function needs to be split in two. - initializeStorageGroups(false); + initializeStoragePlanes(false, true); EnableSEHtoExceptionMapping(); setSEHtoExceptionHandler(&abortHandler); diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index 3eea4441e56..c6fedca40be 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -8821,22 +8821,20 @@ class CConfigUpdater : public CInterface } else if (avoidClone) { + //This is added during the initialiation phase, so no danger of other threads accesing the global information + //while it is updated. Thor currently relies on the pointer not changing. Owned newComponentConfiguration = getComponentConfigSP(); Owned newGlobalConfiguration = getGlobalConfigSP(); refreshConfiguration(newComponentConfiguration, newGlobalConfiguration); } else { - DBGLOG("Refresh %p,", getComponentConfigSP().get()); - dbglogXML(getComponentConfigSP()); // File monitor is disabled - no updates to the configuration files are supported. //So clone the existing configuration and use that to refresh the config - update fucntions may perform differently. Owned newComponentConfiguration = createPTreeFromIPT(getComponentConfigSP()); Owned newGlobalConfiguration = createPTreeFromIPT(getGlobalConfigSP()); refreshConfiguration(newComponentConfiguration, newGlobalConfiguration); } - DBGLOG("Refreshed %p,", getComponentConfigSP().get()); - dbglogXML(getComponentConfigSP()); } void executeCallbacks() @@ -8873,7 +8871,7 @@ class CConfigUpdater : public CInterface } return notifyFuncId; } - unsigned addModifyFunc(ConfigModifyFunc notifyFunc) + unsigned addModifyFunc(ConfigModifyFunc notifyFunc, bool threadSafe) { CriticalBlock b(notifyFuncCS); notifyFuncId++; @@ -8883,8 +8881,7 @@ class CConfigUpdater : public CInterface //Force all cached values to be recalculated, do not reload the config //This is only legal if no other threads are accessing the config yet - otherwise the reading thread //could crash when the global configuration is updated. - //MORE: This function needs an extra parameter to indicate whether or not threads have already started/avoid clone - refreshConfiguration(false, true); + refreshConfiguration(false, threadSafe); DBGLOG("Modify functions should be registered before the configuration is loaded"); } return notifyFuncId; @@ -8925,11 +8922,11 @@ unsigned installConfigUpdateHook(ConfigUpdateFunc notifyFunc, bool callWhenInsta return configFileUpdater->addNotifyFunc(notifyFunc, callWhenInstalled); } -jlib_decl unsigned installConfigUpdateHook(ConfigModifyFunc notifyFunc) // This function must be called before the configuration is loaded. +jlib_decl unsigned installConfigUpdateHook(ConfigModifyFunc notifyFunc, bool threadSafe) // This function must be called before the configuration is loaded. { if (!configFileUpdater) // NB: installConfigUpdateHook should always be called after configFileUpdater is initialized return 0; - return configFileUpdater->addModifyFunc(notifyFunc); + return configFileUpdater->addModifyFunc(notifyFunc, threadSafe); } void removeConfigUpdateHook(unsigned notifyFuncId) @@ -8973,7 +8970,7 @@ void CConfigUpdateHook::installOnce(ConfigUpdateFunc callbackFunc, bool callWhen } -void CConfigUpdateHook::installOnce(ConfigModifyFunc callbackFunc) +void CConfigUpdateHook::installModifierOnce(ConfigModifyFunc callbackFunc, bool threadSafe) { unsigned id = configCBId.load(std::memory_order_acquire); if ((unsigned)-1 == id) // avoid CS in common case @@ -8983,7 +8980,7 @@ void CConfigUpdateHook::installOnce(ConfigModifyFunc callbackFunc) id = configCBId.load(std::memory_order_acquire); if ((unsigned)-1 == id) { - id = installConfigUpdateHook(callbackFunc); + id = installConfigUpdateHook(callbackFunc, threadSafe); configCBId.store(id, std::memory_order_release); } } @@ -10231,8 +10228,6 @@ void setExpertOpt(const char *opt, const char *value) config->setPropTree(xpath); getExpertOptPath(opt, xpath.clear()); config->setProp(xpath, value); - DBGLOG("*** SET *** %p", config.get()); - dbglogXML(config); } //--------------------------------------------------------------------------------------------------------------------- diff --git a/system/jlib/jptree.hpp b/system/jlib/jptree.hpp index 96b4ea1fd85..cf985f29ac6 100644 --- a/system/jlib/jptree.hpp +++ b/system/jlib/jptree.hpp @@ -346,7 +346,7 @@ class jlib_decl CConfigUpdateHook ~CConfigUpdateHook() { clear(); } void clear(); void installOnce(ConfigUpdateFunc callbackFunc, bool callWhenInstalled); - void installOnce(ConfigModifyFunc callbackFunc); + void installModifierOnce(ConfigModifyFunc callbackFunc, bool threadSafe); }; /* diff --git a/thorlcr/master/thmastermain.cpp b/thorlcr/master/thmastermain.cpp index f74a34cfdfa..16f2df64d02 100644 --- a/thorlcr/master/thmastermain.cpp +++ b/thorlcr/master/thmastermain.cpp @@ -398,8 +398,6 @@ class CRegistryServer : public CSimpleInterface processGroup->serialize(msg); globals->serialize(msg); getGlobalConfigSP()->serialize(msg); - DBGLOG("**** CONFIG ****"); - dbglogXML(globals); msg.append(managerWorkerMpTag); msg.append(kjServiceMpTag); if (!queryNodeComm().send(msg, RANK_ALL_OTHER, MPTAG_THORREGISTRATION, MP_ASYNC_SEND)) @@ -762,7 +760,7 @@ int main( int argc, const char *argv[] ) } //This can only be called once dali is initialised - initializeStorageGroups(true); + initializeStoragePlanes(true, true); if (globals->getPropBool("@MPChannelReconnect")) getMPServer()->setOpt(mpsopt_channelreopen, "true");