Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HPCC-30365 Add XREF Sasha service to K8s #18798

Draft
wants to merge 23 commits into
base: candidate-9.6.x
Choose a base branch
from
Draft

HPCC-30365 Add XREF Sasha service to K8s #18798

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
62d12fa
HPCC-30365 Add XREF Sasha service to K8s
jackdelv Apr 25, 2024
1f2b539
Add constructPartFilename to CLogicalNameEntry
jackdelv Jul 8, 2024
4cceb25
Check if running on localhost before setting Dafs port to 0.
jackdelv Jul 8, 2024
525f982
Change error message to report missing storage planes rather than clu…
jackdelv Jul 8, 2024
757a618
Move group combination to BM only.
jackdelv Jul 9, 2024
4b0b9c7
Common up XRefNodes code with containerized case
jackdelv Jul 9, 2024
18d94ab
Add call to calcStripeNumber and use makePhysucalPartName
jackdelv Jul 10, 2024
f340ccc
Use lfnHash from file metadata instead of logical filename
jackdelv Jul 11, 2024
0f9ba2e
Remove ifdef _CONTAINERIZED from constructPartFilename
jackdelv Aug 6, 2024
b0561b2
Fix dir-per-part and stripNum masks in parseFileName.
jackdelv Aug 6, 2024
6ec8287
Remove Containerized DBGLOG message from generalized BM code in onDFU…
jackdelv Aug 8, 2024
43d9d89
Co-locate fullname closer to makePhysicalPartName
jackdelv Aug 8, 2024
15eaf5d
Properly check for dirPerPart in file flags.
jackdelv Aug 8, 2024
e0b61ca
Add comment specifying meaning of localhost in xrefRemoteDirectories
jackdelv Aug 8, 2024
7bc4e0d
Remove changes to runXref and add directory getting code to CXRefMana…
jackdelv Aug 9, 2024
3af5870
Merge branch 'candidate-9.6.x' into xrefContainerized
jackdelv Aug 20, 2024
72fe732
Remove use of addUniqueXrefNode
jackdelv Nov 6, 2024
33bb654
Add example options to xref
jackdelv Nov 6, 2024
44fd255
Move constructPartFilename funtion to dadfs.cpp
jackdelv Nov 7, 2024
2a463a8
Change to makeStringException
jackdelv Nov 7, 2024
ded8478
Treat paths as case sensitive
jackdelv Nov 7, 2024
5aa25a9
Match file prefix to storage plane definition
jackdelv Nov 8, 2024
7fe148a
Add isContainerized() checks
jackdelv Nov 8, 2024
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
93 changes: 4 additions & 89 deletions dali/base/dadfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,10 @@ RemoteFilename &constructPartFilename(IGroup *grp,unsigned partno,unsigned partm
StringBuffer fullname;
if (findPathSepChar(name)==NULL)
addPathSepChar(fullname.append(partdir));
#ifdef _CONTAINERIZED
Copy link
Member

Choose a reason for hiding this comment

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

in general, using isContainerized() vs the #define is preferrable (so that the code is scanned/compiled by developers routinely etc.)
And, if both can be avoided and the code made general (even if the use case is not currently used in bare-metal), that is most preferrable.

This is a case in point, we should cope with this even in the bare-metal case in general utility classes/methods, even though it is not configurable in BM at the moment.

But also, it looks like this exposes a wide problem(!). This function is used not just by XREF, it's also used by dadfs.cpp removePhysicalFiles

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just noticed that removePhysicalFiles is not used by anything - we should delete it avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect constructPartFilename should be deprecated (probably removed too).
In XREF's case, it needs a way to get a physical part + endpoint given the meta data it's collected.

The 'lnentry' (CLogicalNameEntry) it has built from meta data, should perhaps have a method to built it, it should know (from the meta data) if the file is striped and if it has dirPerPart.

Btw, dirPerPart (a flag in the meta data) means it has a directory extension that includes the file part number (i.e. each part is in it's own directory)
'striping' is different, and an additional directory not per part, but enumerated over N striped disks. The stripNum is calculated from a hash (@lfnHash) stored in the file meta. Originally calculated and set there by hashing the logicalfilename.

See makePhysicalPartName and it's callers (and uses of lfnHash and calcStripeNumber).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed remotePhysicalFiles and added a constructPartFilename function to CLogicalNameEntry. I had some trouble finding the dirPerPart flag in the metadata. The only examples I was able to find was using queryDirPerPart() on a StoragePlane, but when trying that it seemed the metadata was missing and it was defaulting to the return value of isContainerized().

Added lfnHash to the CLogicalNameEntry and used calcStripeNumber on the part number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old constructPartFilename function had some tests for it, so I couldn't delete the function without also removing the tests. Should there now be tests for the new constructPartFilename if it does get added to CLogicalNameEntry?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should remove the old constructPartFilename, that is now only used by tests, and if not covered already, add tests that exercise makePhysicalPartName.

But for now, for this PR, I would just add comments that it is deprecated, but test code uses it - and open a separate JIRA to revisit sometime.

Copy link
Member

Choose a reason for hiding this comment

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

Given the new CLogicalNameEntry::constructPartFilename, I think you can remove the changes here right? i.e.:

#ifdef _CONTAINERIZED
    if (partmax>1)
        addPathSepChar(fullname.append(partno+1)); // If there are multiple files they will be striped by part number
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the _Containerized changes and added a comment to the old constructPartFilename.

if (partmax>1)
addPathSepChar(fullname.append(partno+1)); // If there are multiple files they will be striped by part number
#endif
fullname.append(name);
unsigned n;
unsigned d;
Expand Down Expand Up @@ -12008,95 +12012,6 @@ void CDistributedFileDirectory::setDefaultPreferredClusters(const char *clusters
defprefclusters.set(clusters);
}

bool removePhysicalFiles(IGroup *grp,const char *_filemask,unsigned short port,ClusterPartDiskMapSpec &mspec,IMultiException *mexcept)
{
// TBD this won't remove repeated parts


PROGLOG("removePhysicalFiles(%s)",_filemask);
if (!isAbsolutePath(_filemask))
throw MakeStringException(-1,"removePhysicalFiles: Filename %s must be complete path",_filemask);

size32_t l = strlen(_filemask);
while (l&&isdigit(_filemask[l-1]))
l--;
unsigned width=0;
if (l&&(_filemask[l-1]=='_'))
width = atoi(_filemask+l);
if (!width)
width = grp->ordinality();

CriticalSection errcrit;
class casyncfor: public CAsyncFor
{
unsigned short port;
CriticalSection &errcrit;
IMultiException *mexcept;
unsigned width;
StringAttr filemask;
IGroup *grp;
ClusterPartDiskMapSpec &mspec;
public:
bool ok;
casyncfor(IGroup *_grp,const char *_filemask,unsigned _width,unsigned short _port,ClusterPartDiskMapSpec &_mspec,IMultiException *_mexcept,CriticalSection &_errcrit)
: mspec(_mspec),filemask(_filemask),errcrit(_errcrit)
{
grp = _grp;
port = _port;
ok = true;
mexcept = _mexcept;
width = _width;
}
void Do(unsigned i)
{
for (unsigned copy = 0; copy < 2; copy++) // ** TBD
{
RemoteFilename rfn;
constructPartFilename(grp,i+1,width,NULL,filemask,"",copy>0,mspec,rfn);
if (port)
rfn.setPort(port); // if daliservix
Owned<IFile> partfile = createIFile(rfn);
StringBuffer eps;
try
{
unsigned start = msTick();
#if 1
if (partfile->remove()) {
PROGLOG("Removed '%s'",partfile->queryFilename());
unsigned t = msTick()-start;
if (t>5*1000)
DBGLOG("Removing %s from %s took %ds", partfile->queryFilename(), rfn.queryEndpoint().getEndpointHostText(eps).str(), t/1000);
}
else
IWARNLOG("Failed to remove file part %s from %s", partfile->queryFilename(),rfn.queryEndpoint().getEndpointHostText(eps).str());
#else
if (partfile->exists())
PROGLOG("Would remove '%s'",partfile->queryFilename());
#endif

}
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().getEndpointHostText(s);
EXCLOG(e, s.str());
e->Release();
}
ok = false;
}
}
}
} afor(grp,_filemask,width,port,mspec,mexcept,errcrit);
afor.For(width,10,false,true);
return afor.ok;
}


IDaliServer *createDaliDFSServer(IPropertyTree *config)
{
assertex(!daliDFSServer); // initialization problem
Expand Down
3 changes: 0 additions & 3 deletions dali/base/dadfs.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,9 +828,6 @@ extern da_decl IDFPartFilter *createPartFilter(const char *filter);
| <part_number> '-' <part_number>
*/

extern da_decl bool removePhysicalFiles(IGroup *grp,const char *_filemask,unsigned short port, ClusterPartDiskMapSpec &mspec,IMultiException *mexcept);
// for removing orphaned files

// for server use
interface IDaliServer;
extern da_decl IDaliServer *createDaliDFSServer(IPropertyTree *config); // called for coven members
Expand Down
Loading
Loading