search

toddproject / todd

A distributed network assurance platform
Apache License 2.0
237 stars 30 forks source link

Removed unnecessary 'continue' in agent advert loop #151

Closed Mierdin closed 5 years ago

Mierdin commented 7 years ago

The continue statement in the agent advertisement loop didn't seem to be there for good reason. I made a change to one of the fact collectors which caused the incoming fact to not be compatible with the struct that it gets imported into, and as a result, this code ran. However, the continue would cause the rest of the code to not be run, including the sleep, so it wouldn't do much of anything but fill up logs....quickly.

So this fixes that, I don't think this continue serves any functional purpose.

vcabbage commented 7 years ago

I think it depends on whether it's expected/useful for an agent to advertise itself with a nil list of facts. I assumed that wasn't a valid situation, perhaps that's wrong?

It was an oversight on my part that it doesn't sleep after an error.

Mierdin commented 7 years ago

I'm okay with advertising whatever facts.GetFacts returns, but yeah if err != nil, that value will likely not be worth much. So perhaps we just move the sleep to the top and add the continue back in.

vcabbage commented 7 years ago

I think moving the sleep to the top of the loop will delay agents becoming available by 10s, which seems unfortunate. It might make sense to move the logic into a separate function, calling that function once outside the loop, then entering the loop with the sleep at the beginning.

Mierdin commented 5 years ago

I elected to move GetFacts outside the loop and be done with it. In the upcoming rebuild effort, this logic won't matter anymore, so I'm not sweating it too much for now. Merging