-
Notifications
You must be signed in to change notification settings - Fork 304
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.
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
HPCC-31530 Remove stale LZ groups #18458
HPCC-31530 Remove stale LZ groups #18458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakesmith looks like a good change. A couple of suggestions for improving reuse/optimizing it.
throw makeStringException(0, "CInitGroups::clearLZGroups called in read-only mode"); | ||
IPropertyTree *root = groupsconnlock.conn->queryRoot(); | ||
std::vector<Owned<IPropertyTree>> toDelete; | ||
Owned<IPropertyTreeIterator> groups = root->getElements("Group[@kind='dropzone']"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root->removeElements().. might be quite a useful function - even if it was implemented with this code, rather than being optimized. (That could come later.)
I had a quick search, and there are quite a few chunks of code that follow this pattern (e.g. addJsonPublicService), but there are also several that remove the tree within the iterator. Will that work? (E.g. removeEclHiddenStructs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are also several that remove the tree within the iterator. Will that work? (E.g. removeEclHiddenStructs)
I think it should work, the iterators link elements, or continue to use ptrees + xpaths, but I wouldn't advise deleting items from an active iterator.
Good idea to enter a dedicated method, but it and refactoring code that uses it, yes in a new PR. I'll open a JIRA.
dali/base/dadfs.cpp
Outdated
if (!writeLock) | ||
throw makeStringException(0, "CInitGroups::clearLZGroups called in read-only mode"); | ||
IPropertyTree *root = groupsconnlock.conn->queryRoot(); | ||
std::vector<Owned<IPropertyTree>> toDelete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A vector of IPropertyTree * and iter->query() would avoid linking and releasing the tree items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, I'll change it.
@ghalliday - please see new commit. Let me know if you are happy for me to squash it. |
Remove existing Dali LZ groups before creating groups from the environment or storage planes. This avoids stale groups being left behind, which in-turn can cause systems to spuriously see them and try to reference nodes that no longer exist, causing stalls and delays to be introduced. Signed-off-by: Jake Smith <[email protected]>
3722612
to
8ce96c9
Compare
Remove existing Dali LZ groups before creating groups from the environment or storage planes.
This avoids stale groups being left behind, which in-turn can cause systems to spuriously see them and try to reference nodes that no longer exist, causing stalls and delays to be introduced.
Type of change:
Checklist:
Smoketest:
Testing: