QA (public) » openQA Project (public)

Quoting Ettore

dasantiago wrote:

My changes only adds a dependency that was missing, so because of that change it shouldn't lose anything.

Yes and no, as it's not only adding, before it was just skipping - if you look closely at https://github.com/os-autoinst/openQA/pull/1623/files#diff-85ae48e70a5c110c9e439c3a5ea28d5fR759 you can also skip other child deps ( see next() ) and ignore the other conditions down while cycling, that could call duplicate (recursively, again) and i suspect that can bring also to loose duplicated jobs dependencies; even if unpleasant, duplicate() looks buggy, to fix properly this we would need to rewrite it from scratch

For example:
https://openqa.suse.de/tests/1661011#settings

This probably was introduced with my change, but the losing deps? :-(

Is there any multimachine test environment that i can use for my needs? Or is there any way to simulate a multi machine environment?

szarate wrote:

Quoting Ettore

dasantiago wrote:

My changes only adds a dependency that was missing, so because of that change it shouldn't lose anything.

Yes and no, as it's not only adding, before it was just skipping - if you look closely at https://github.com/os-autoinst/openQA/pull/1623/files#diff-85ae48e70a5c110c9e439c3a5ea28d5fR759 you can also skip other child deps.

The next is to avoid duplicating jobs already duplicated. Please check the if condition https://github.com/os-autoinst/openQA/pull/1623/files#diff-85ae48e70a5c110c9e439c3a5ea28d5fR750

Also if you compare with the previous commit, it's the same condition that launches the next.

The function implements the depth first search algorithm https://en.wikipedia.org/wiki/Depth-first_search

dasantiago wrote:

szarate wrote:

Quoting Ettore

dasantiago wrote:

My changes only adds a dependency that was missing, so because of that change it shouldn't lose anything.

Yes and no, as it's not only adding, before it was just skipping - if you look closely at https://github.com/os-autoinst/openQA/pull/1623/files#diff-85ae48e70a5c110c9e439c3a5ea28d5fR759 you can also skip other child deps.

The next is to avoid duplicating jobs already duplicated. Please check the if condition https://github.com/os-autoinst/openQA/pull/1623/files#diff-85ae48e70a5c110c9e439c3a5ea28d5fR750

Also if you compare with the previous commit, it's the same condition that launches the next.

Yes, i believe the point is the recursiveness impact of your changes

The function implements the depth first search algorithm https://en.wikipedia.org/wiki/Depth-first_search

Which is one of the basis of graph search algorithms, i would not guess anything different to going on there - but i guess you already know that since you are linking me a wikipedia page :)

If it did correctly then we didn't see such problems - also, we should cover it with extracting the graph algorithm then from the schema / and having separate test for the specific function, but i'm afraid it's not just a simple plain implementation so i know it's harder than it sounds.

Also as you can see yourself from DFS, it can be implemented in iterative way instead of using recursion :)

But in graph algorithms there are a lot of ways to achieve what we need here, like dynamic programming techniques would help a lot to reduce mostly the domain of our problem since we don't have big graphs to handle - what i'm saying here is simply: we should refactor it, decouple it from schema so we can actually test it, and then apply it to the real schema.

EDiGiacinto wrote:

Which is one of the basis of graph search algorithms, i would not guess anything different to going on there - but i guess you already know that since you are linking me a wikipedia page :)

I just added more info into the ticket, as someone might have trouble to understand the algorithm/code.

If it did correctly then we didn't see such problems

This is were i don't totally agree with you. I'm not saying that the code is perfect and it doesn't contains bugs, but the issue existed for some time and since this seems to happen only in MM jobs, I'm a bit more inclined to think that there's a problem somewhere else - But it's only my opinion

Also as you can see yourself from DFS, it can be implemented in iterative way instead of using recursion :)

Everything can be implemented without using recursion. Using recursion is just convenient in some cases. Looping through graph nodes is one of those cases :-)

we should refactor it, decouple it from schema so we can actually test it, and then apply it to the real schema.

This is where i disagree the most. Because it seems that we should immediately rewrite the code. However i believe that before doing any changes we should start by understanding the issue. Which we don't. Doing changes before understanding what's wrong is usually not a good idea.

dasantiago wrote:

EDiGiacinto wrote:

Which is one of the basis of graph search algorithms, i would not guess anything different to going on there - but i guess you already know that since you are linking me a wikipedia page :)

I just added more info into the ticket, as someone might have trouble to understand the algorithm/code.

If it did correctly then we didn't see such problems

This is were i don't totally agree with you. I'm not saying that the code is perfect and it doesn't contains bugs, but the issue existed for some time and since this seems to happen only in MM jobs, I'm a bit more inclined to think that there's a problem somewhere else - But it's only my opinion

I'm not saying that it's bugfree the whole function, but just that recursion imho bite your change in unwanted sides.

Also as you can see yourself from DFS, it can be implemented in iterative way instead of using recursion :)

Everything can be implemented without using recursion. Using recursion is just convenient in some cases. Looping through graph nodes is one of those cases :-)

Of course it's convenient - iteration here possibly would have the function easier to read. (not even talking about performance impact here)

we should refactor it, decouple it from schema so we can actually test it, and then apply it to the real schema.

This is where i disagree the most. Because it seems that we should immediately rewrite the code. However i believe that before doing any changes we should start by understanding the issue. Which we don't. Doing changes before understanding what's wrong is usually not a good idea.

I feel here you are scared about touching the code? If something requires to be re-thought, then i think we should do when necessary - imho this code section calls a proper rewrital/implementation.

Indeed refactoring is not about changing the same function, but write it from scratch with a good testing unit right on the side of it (TDD ftw) , not later in time. Extracting into functions in this case just make it easier to test it.

So I managed to extract the lookup with tests passing only, but as soon as I try to remove the recursion by simply building the dependency tree in one shot, it all goes down south pretty fast.