-
Notifications
You must be signed in to change notification settings - Fork 582
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
Dependency: track all relationships via DependencyGraph #8197
Dependency: track all relationships via DependencyGraph #8197
Conversation
BeforeAfter
|
Hi, any updates on that? looking forward to seeing that merged. |
... to ensure dependencies get deleted via cascade=1. refs #8180
b6c2fc6
to
556d4d2
Compare
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.
I think there was just a typo (or copy paste mistake) in the old code, see inline comment. Fixing this should suffice, other places use the very same method to track dependencies, see for example comment.ti
:
Lines 37 to 61 in ca52366
[config, protected, required, navigation(host)] name(Host) host_name { | |
navigate {{{ | |
return Host::GetByName(GetHostName()); | |
}}} | |
}; | |
[config, protected, navigation(service)] String service_name { | |
track {{{ | |
if (!oldValue.IsEmpty()) { | |
Service::Ptr service = Service::GetByNamePair(GetHostName(), oldValue); | |
DependencyGraph::RemoveDependency(this, service.get()); | |
} | |
if (!newValue.IsEmpty()) { | |
Service::Ptr service = Service::GetByNamePair(GetHostName(), newValue); | |
DependencyGraph::AddDependency(this, service.get()); | |
} | |
}}} | |
navigate {{{ | |
if (GetServiceName().IsEmpty()) | |
return nullptr; | |
Host::Ptr host = Host::GetByName(GetHostName()); | |
return host->GetServiceByShortName(GetServiceName()); | |
}}} | |
}; |
So if there's something wrong in general with how tracking of service dependencies works right now, there are more places where this should be changes. But I think the current way works fine apart from the typo in the tracking code for Dependency.child_service_name
.
Note that there is no tracking explicitly specified, this is implicitly generated (there is ObjectImpl<Comment>::TrackHostName
in comment-ti.cpp
for example).
navigate {{{ | ||
return Host::GetByName(GetChildHostName()); | ||
}}} | ||
}; | ||
|
||
[config, navigation(child_service)] String child_service_name { | ||
track {{{ | ||
if (!oldValue.IsEmpty()) { | ||
Service::Ptr service = Service::GetByNamePair(GetParentHostName(), oldValue); |
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.
I think the root cause for the related issue is that it should say GetChildHostName()
here and in line 40.
... to ensure dependencies get deleted via cascade=1.
fixes #8180