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

DependencyGraph: switch "parent" and "child" terminology #10263

Merged
merged 1 commit into from
Dec 12, 2024
Merged

DependencyGraph: switch "parent" and "child" terminology #10263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions lib/base/dependencygraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@ using namespace icinga;
std::mutex DependencyGraph::m_Mutex;
std::map<Object *, std::map<Object *, int> > DependencyGraph::m_Dependencies;

void DependencyGraph::AddDependency(Object *parent, Object *child)
void DependencyGraph::AddDependency(Object* child, Object* parent)
{
std::unique_lock<std::mutex> lock(m_Mutex);
m_Dependencies[child][parent]++;
m_Dependencies[parent][child]++;
}

void DependencyGraph::RemoveDependency(Object *parent, Object *child)
void DependencyGraph::RemoveDependency(Object* child, Object* parent)
{
std::unique_lock<std::mutex> lock(m_Mutex);

auto& refs = m_Dependencies[child];
auto it = refs.find(parent);
auto& refs = m_Dependencies[parent];
auto it = refs.find(child);

if (it == refs.end())
return;
Expand All @@ -29,15 +29,15 @@ void DependencyGraph::RemoveDependency(Object *parent, Object *child)
refs.erase(it);

if (refs.empty())
m_Dependencies.erase(child);
m_Dependencies.erase(parent);
}

std::vector<Object::Ptr> DependencyGraph::GetParents(const Object::Ptr& child)
std::vector<Object::Ptr> DependencyGraph::GetChildren(const Object::Ptr& parent)
{
std::vector<Object::Ptr> objects;

std::unique_lock<std::mutex> lock(m_Mutex);
auto it = m_Dependencies.find(child.get());
auto it = m_Dependencies.find(parent.get());

if (it != m_Dependencies.end()) {
typedef std::pair<Object *, int> kv_pair;
Expand Down
6 changes: 3 additions & 3 deletions lib/base/dependencygraph.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ namespace icinga {
class DependencyGraph
{
public:
static void AddDependency(Object *parent, Object *child);
static void RemoveDependency(Object *parent, Object *child);
static std::vector<Object::Ptr> GetParents(const Object::Ptr& child);
static void AddDependency(Object* child, Object* parent);
static void RemoveDependency(Object* child, Object* parent);
static std::vector<Object::Ptr> GetChildren(const Object::Ptr& parent);

private:
DependencyGraph();
Expand Down
2 changes: 1 addition & 1 deletion lib/base/scriptutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ String ScriptUtils::MsiGetComponentPathShim(const String& component)

Array::Ptr ScriptUtils::TrackParents(const Object::Ptr& child)
{
return Array::FromVector(DependencyGraph::GetParents(child));
return Array::FromVector(DependencyGraph::GetChildren(child));
Copy link
Member

Choose a reason for hiding this comment

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

The method is called TrackParents and not children, how is this supposed to work now?

<4> => get_object(Dependency, "brother!mother")
{
	__name = "brother!mother"
	active = true
	child_host_name = "brother"
	child_service_name = ""
	disable_checks = false
	disable_notifications = true
        ....
<5> => track_parents(get_object(Dependency, "brother!mother"))
[ ]
<8> => track_parents(get_host("mother"))
[ {
	__name = "brother!mother"
	active = true
	child_host_name = "brother"
	child_service_name = ""
	disable_checks = false
	disable_notifications = true
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it has never worked, because I literally just renamed methods and method args.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to also fix this, it would be a breaking change. (I'd even drop the method, so that its users notice easily.) But this is something for v2.15, i.e another PR. (And no, I won't open another PR right now, don't worry.)

Copy link
Member

Choose a reason for hiding this comment

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

If @julianbrost wants to merge this while that DSL function is still broken (was broken before too), then it would probably work as expected with #10000like for the first time since it was introduced :).

Copy link
Contributor

Choose a reason for hiding this comment

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

That function looks like a early debugging left-over to me. It's not mentioned in the documentation and from the name track_parents it isn't really clear what it does.

(I'd even drop the method, so that its users notice easily.) But this is something for v2.15

Sounds like what I would do. (If we figure out that there's a real-world use for it, it should probably get a proper name and documentation.)

Copy link
Member Author

Choose a reason for hiding this comment

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

And the best discovery tool for real-world usages is removal.😈

Copy link
Member Author

Choose a reason for hiding this comment

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

But yes, your fact checking is correct:

$ git grep -nFie track_parents
lib/base/scriptutils.cpp:52:REGISTER_SAFE_FUNCTION(System, track_parents, &ScriptUtils::TrackParents, "child");
$ git grep -nFie track\\_parents
$ git grep -nFie track\\\\_parents
$

Same at Google btw.: https://letmegooglethat.com/?q=%22icinga%22+2+%22track_parents%22

Copy link
Member

Choose a reason for hiding this comment

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

Yay 🎉! It works now with d28dfec!

<11> => track_parents(get_object(Dependency, "brother!mother"))
[ {
	__name = "mother"
	acknowledgement = 0.000000
	acknowledgement_expiry = 0.000000
	acknowledgement_last_change = 0.000000
	action_url = ""
	active = true
	address = ""
	address6 = ""
	check_attempt = 1.000000
	check_command = "dummy"
	check_interval = 60.000000
	check_period = ""
	check_timeout = null
	command_endpoint = ""
	display_name = "mother"
	downtime_depth = 0.000000
	enable_active_checks = true
	enable_event_handler = true
	enable_flapping = false
	enable_notifications = true
	enable_passive_checks = true
....

}

double ScriptUtils::Ptr(const Object::Ptr& object)
Expand Down
2 changes: 1 addition & 1 deletion lib/remote/configobjectutility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
bool ConfigObjectUtility::DeleteObjectHelper(const ConfigObject::Ptr& object, bool cascade,
const Array::Ptr& errors, const Array::Ptr& diagnosticInformation, const Value& cookie)
{
std::vector<Object::Ptr> parents = DependencyGraph::GetParents(object);
auto parents (DependencyGraph::GetChildren(object));

Type::Ptr type = object->GetReflectionType();

Expand Down
2 changes: 1 addition & 1 deletion lib/remote/objectqueryhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ bool ObjectQueryHandler::HandleRequest(
Array::Ptr used_by = new Array();
metaAttrs.emplace_back("used_by", used_by);

for (const Object::Ptr& pobj : DependencyGraph::GetParents((obj)))
for (auto& pobj : DependencyGraph::GetChildren(obj))
{
ConfigObject::Ptr configObj = dynamic_pointer_cast<ConfigObject>(pobj);

Expand Down
Loading