HPCC-36100 Implement directoryFiles for Azure Blob and Files classes#21188
HPCC-36100 Implement directoryFiles for Azure Blob and Files classes#21188jackdelv wants to merge 4 commits intohpcc-systems:candidate-10.0.xfrom
Conversation
🔄 Upmerge Test ResultsStatus: ✅ All branches merged successfully ✅ Successful Branches (2)
|
ghalliday
left a comment
ghalliday
left a comment
There was a problem hiding this comment.
First view looks good. A few minor comments and questions.
| // Subclass creates the appropriate IFile (AzureBlob or AzureFile) for the given path | ||
| virtual IFile *createFile(const char *fullPath, const DirEntry &entry) = 0; | ||
|
|
||
| bool matchesMask(const char *name) const { return !mask.length() || WildMatch(name, mask, false); } |
There was a problem hiding this comment.
minor: Use mask.isEmpty() rather than !mask.length(). Calling length() will perform a strlen(), isEmpty will only check the first character. Possibly worth having a member variable isFiltered which can be set up once.
There was a problem hiding this comment.
Changed to isEmpty()
| } | ||
| } | ||
| const DirEntry &entry = items[itemIndex]; | ||
| curName.set(entry.name.c_str()); |
There was a problem hiding this comment.
Avoid cloning these values - cloning the names could be quite expensive. The others are not so significant, but worth avoiding. Could have a function like this to simplify the calling code.
inline const DirEntry & queryCurEntry() const { return items[itemIndex]; }
There was a problem hiding this comment.
Remove cloned member variables.
| ListBlobsOptions options; | ||
| if (!prefix.empty()) | ||
| options.Prefix = prefix; | ||
| pagedResponse.emplace(containerClient->ListBlobsByHierarchy("/", options)); |
There was a problem hiding this comment.
For heirarchical namespaces does this return all files in all subdirectories, or only direct subfiles?
There was a problem hiding this comment.
It only returns direct subfiles
|
|
||
| IFile * AzureBlobDirectoryIterator::createFile(const char *fullPath, const DirEntry &entry) | ||
| { | ||
| AzureBlob *file = static_cast<AzureBlob *>(createAzureBlob(fullPath)); |
There was a problem hiding this comment.
Would this be cleaner as a constructor for AzureBlob which was also passed a DirEntry? It does not need to be exported.
There was a problem hiding this comment.
Yes, I think so. Added a constructor overload that calls setListedInfo.
🔄 Upmerge Test ResultsStatus: ✅ All branches merged successfully ✅ Successful Branches (3)
|
2 participants
Type of change:
Checklist:
Smoketest:
Testing: