Skip to content
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 76 additions & 5 deletions dali/dfuXRefLib/XRefNodeManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include "jstring.hpp"
#include "jptree.hpp"
#include "jmisc.hpp"
#include "jfile.hpp"
#include "jutil.hpp"

#include "mpcomm.hpp"
#include "platform.h"
Expand Down Expand Up @@ -120,12 +122,81 @@ CXRefNode::CXRefNode(IPropertyTree* pTreeRoot)
m_XRefTree.set(pTreeRoot);
rootDir.set(m_XRefTree->queryProp("@rootdir"));
pTreeRoot->getProp("@name",m_origName);
//load up our tree with the data.....if there is data
MemoryBuffer buff;
pTreeRoot->getPropBin("data",buff);
if (buff.length())

// Check if path metadata is available (new Sasha plane storage)
const char *xrefPath = pTreeRoot->queryProp("@xrefPath");
if (xrefPath && *xrefPath)
{
m_dataStr.append(buff.length(),buff.toByteArray());
// New path-based storage - load branches from files
try
{
StringBuffer basePath(xrefPath);
// Handle file:// URLs
if (hasPrefix(basePath, "file://", false))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - this is not necessary, remove this block.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Removed file:// URL handling block entirely.

{
// Extract path from file://hostname/path format
const char *pathStart = basePath.str() + 7; // Skip "file://"
// Find the next slash which marks the start of the actual path
const char *pathSep = strchr(pathStart, '/');
if (pathSep)
{
basePath.clear().append(pathSep);
}
}

// Load each branch from its file
const char *branchNames[] = {"Orphans", "Lost", "Found", "Directories", "Messages", nullptr};
for (int i = 0; branchNames[i] != nullptr; i++)
{
StringBuffer filepath(basePath);
addPathSepChar(filepath).append(branchNames[i]).append(".xml");

Owned<IFile> file = createIFile(filepath.str());
if (file->exists())
{
Owned<IFileIO> fileIO = file->open(IFOread);
if (fileIO)
{
offset_t fileSize = file->size();
if (fileSize > 0 && fileSize < 0x10000000) // Sanity check: < 256MB
{
StringBuffer xmlContent;
xmlContent.ensureCapacity((size32_t)fileSize);
size32_t bytesRead = fileIO->read(0, (size32_t)fileSize, (void*)xmlContent.reserve((size32_t)fileSize));
if (bytesRead > 0)
{
Owned<IPropertyTree> branch = createPTreeFromXMLString(xmlContent.str());
if (branch)
{
m_XRefTree->addPropTree(branchNames[i], branch.getClear());
}
}
}
}
}
}
DBGLOG("XRefNode: Loaded XREF data from path: %s", basePath.str());
}
catch (IException *e)
{
StringBuffer errMsg;
OWARNLOG("XRefNode: Failed to load from path '%s': %s, falling back to 'data' attribute",
xrefPath, e->errorMessage(errMsg).str());
e->Release();
// Fall through to load from data attribute
}
}

// Fall back to loading from "data" attribute if path not available or failed
if (!m_XRefTree->hasProp("Orphans") && !m_XRefTree->hasProp("Lost") &&
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - don't failover like this, simply make this the else branch if xrefpath is missing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Made it a simple else branch instead of fallback logic.

!m_XRefTree->hasProp("Found") && !m_XRefTree->hasProp("Directories"))
{
MemoryBuffer buff;
pTreeRoot->getPropBin("data",buff);
if (buff.length())
{
m_dataStr.append(buff.length(),buff.toByteArray());
}
}
//lets check to ensure we have the correct children inplace(Orphan,lost,found)
}
Expand Down
156 changes: 151 additions & 5 deletions dali/sasha/saxref.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
#include "jmisc.hpp"
#include "jregexp.hpp"
#include "jset.hpp"
#include "jfile.hpp"
#include "jplane.hpp"
#include "jutil.hpp"
#include "jsocket.hpp"

#include <memory>
#include <unordered_map>
Expand Down Expand Up @@ -1001,6 +1005,40 @@ class CNewXRefManagerBase
heartbeatTimer.updatePeriod();
}

bool saveBranchToSashaPlane(const char *sashaDir, const char *name, IPropertyTree *branch)
{
if (!branch)
return true;
try
{
branch->setProp("Cluster",clustname);
StringBuffer filepath(sashaDir);
addPathSepChar(filepath).append(name).append(".xml");

StringBuffer datastr;
toXML(branch,datastr);

Owned<IFile> file = createIFile(filepath.str());
Owned<IFileIO> fileIO = file->open(IFOcreate);
if (!fileIO)
{
OERRLOG(LOGPFX "Failed to create file: %s", filepath.str());
return false;
}
fileIO->write(0, datastr.length(), datastr.str());
fileIO->close();
PROGLOG(LOGPFX "Saved branch %s to %s", name, filepath.str());
return true;
}
catch (IException *e)
{
StringBuffer errMsg;
EXCLOG(e, LOGPFX "Error saving branch to Sasha plane");
e->Release();
return false;
}
}

void addBranch(IPropertyTree *root,const char *name,IPropertyTree *branch)
{
if (!branch)
Expand Down Expand Up @@ -1086,11 +1124,119 @@ class CNewXRefManagerBase
else if (fnum)
ss.appendf(" [%d files]",fnum);
croot->setProp("@status",ss.str());
addBranch(croot,"Orphans",orphansbranch);
addBranch(croot,"Lost",lostbranch);
addBranch(croot,"Found",foundbranch);
addBranch(croot,"Directories",dirbranch);
addErrorsWarnings(croot);

// Try to use Sasha plane for storage
bool useSashaPlane = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - make this a configurable via an expert option, default to true if isContainerized().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Made configurable via @useSashaPlane expert option, defaults to isContainerized().

StringBuffer sashaDir;
try
{
if (isContainerized())
{
StringBuffer planeName;
// Look for sasha plane
if (getDefaultPlane(planeName, nullptr, "sasha"))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - There should always be a sasha plane categeory defined.
This should throw an error if one cannot be found.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Now throws error if Sasha plane category not found instead of falling back.

{
Owned<const IPropertyTree> sashaPlane = getStoragePlaneConfig(planeName, false);
if (sashaPlane)
{
StringBuffer prefix;
if (sashaPlane->getProp("@prefix", prefix))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - the above calls could be simplified by using getDefaultPlaneDirectory to get the prefix directly (and throwing an error if fails)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Simplified to use getDefaultPlane + getStoragePlaneConfig. Note: getDefaultPlaneDirectory is a static function not available in the header.

{
// Create directory structure: <prefix>/xref/<cluster>/<datestamp>/
sashaDir.append(prefix);
addPathSepChar(sashaDir).append("xref");
addPathSepChar(sashaDir).append(clustname);

// Create datestamp directory
StringBuffer datestamp;
dt.getDateString(datestamp, false); // YYYY-MM-DD format
addPathSepChar(sashaDir).append(datestamp);

// Create the directory
Owned<IFile> dir = createIFile(sashaDir.str());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - use recursiveCreateDirectoryForFile instead and fire an error if it fails.

We should not failover to write to dali.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Now uses recursiveCreateDirectoryForFile and throws error on failure (no fallback to Dali).

if (dir->createDirectory())
{
useSashaPlane = true;
PROGLOG(LOGPFX "Using Sasha plane storage at: %s", sashaDir.str());
}
else
{
OWARNLOG(LOGPFX "Failed to create Sasha directory: %s, falling back to Dali storage", sashaDir.str());
}
}
}
}
}
}
catch (IException *e)
{
StringBuffer errMsg;
OWARNLOG(LOGPFX "Exception getting Sasha plane: %s, falling back to Dali storage", e->errorMessage(errMsg).str());
e->Release();
useSashaPlane = false;
}

if (useSashaPlane)
{
// Save branches to Sasha plane files
bool saveSuccess = true;
saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Orphans", orphansbranch) && saveSuccess;
saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Lost", lostbranch) && saveSuccess;
saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Found", foundbranch) && saveSuccess;
saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Directories", dirbranch) && saveSuccess;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - use 'warn' in saveBranchToSashaPlane to preserve and publish the warning, but don't bother with 'saveSuccess' and a return result.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - saveBranchToSashaPlane now uses warn() and has no return value or saveSuccess tracking.


// Save Messages
Owned<IPropertyTree> message = createPTree("Messages");
ForEachItemIn(i1,errors) {
cMessage &item = errors.item(i1);
IPropertyTree *t = message->addPropTree("Error",createPTree("Error"));
t->addProp("File",item.lname.get());
t->addProp("Text",item.msg.get());
}
ForEachItemIn(i2,warnings) {
cMessage &item = warnings.item(i2);
IPropertyTree *t = message->addPropTree("Warning",createPTree("Warning"));
t->addProp("File",item.lname.get());
t->addProp("Text",item.msg.get());
}
saveSuccess = saveBranchToSashaPlane(sashaDir.str(), "Messages", message) && saveSuccess;

if (saveSuccess)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - see previous comment. Get rid of 'saveSuccess'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Removed saveSuccess tracking completely.

{
// Store path reference in Dali instead of full data
// Convert to hostname-based URL if not a local path
StringBuffer pathUrl;
if (!isAbsolutePath(sashaDir.str()))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot - this in fact is not necessary, simply store the sashaDir in all cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Now stores plain sashaDir path directly (no URL conversion).

{
// If not absolute, make it a URL with hostname
StringBuffer hostname;
queryHostIP().getHostText(hostname);
pathUrl.append("file://").append(hostname).append(sashaDir);
}
else
{
pathUrl.append(sashaDir);
}
croot->setProp("@xrefPath", pathUrl.str());
PROGLOG(LOGPFX "Saved XREF data to Sasha plane with path reference: %s", pathUrl.str());
}
else
{
OWARNLOG(LOGPFX "Failed to save some branches to Sasha plane, falling back to Dali storage");
useSashaPlane = false;
}
}

if (!useSashaPlane)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given we are not going to failover to this. Just make this the else branch of if (useSashaPlane)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c7c4bbb - Made it a simple else branch with no fallback logic.

{
// Fall back to traditional Dali storage
addBranch(croot,"Orphans",orphansbranch);
addBranch(croot,"Lost",lostbranch);
addBranch(croot,"Found",foundbranch);
addBranch(croot,"Directories",dirbranch);
addErrorsWarnings(croot);
}

if (abort)
return;
logconn.clear();
Expand Down