Remove runner creation from every reload check

[vjsamuel]

This PR will partly mitigate #5139 as it removes runner creation during every reload interval.

[elasticmachine]

Can one of the admins verify this patch?

[ruflin]

Travis build failure should be solved by: #5142

[exekias]

Thank you for your contribution @vjsamuel! 🎉

LGTM, I think we should get rid of Runner.ID() method to avoid inconsistencies.

Also, want to hear from @urso as he was involved in this

[ruflin]

Left one comment as I remember there was a reason for the ID(). Good thing is that all tests seem to pass :-)

It seems this would only mitigate the memory issue, so really keen on figuring out what the actual issue is too.

[ruflin]

Not 100% sure anymore, but I think there is a difference between this and ID(). In the case of ID() the following to configs are the same:

a: b
c: d

and

c: d
a: b

That means the order does not matter, but with the above it does. But not 100% sure anymore if that was the reason I ended up with ID() but there was an issue.

[vjsamuel]

func main () {
	a := map[string]interface{}{
		"a": "b",
		"c": "d",
	}


	b := map[string]interface{}{
		"c": "d",
		"a": "b",
	}

	fmt.Println(hashstructure.Hash(a, nil))
	fmt.Println(hashstructure.Hash(b, nil))
}

output:

8710980024335696864 <nil>
8710980024335696864 <nil>

[ruflin]

BTW: If we can remove the ID method 👍 from my side. I think also @urso did not like it when I introduced it but we didn't have a better way. That is why I worry the above breaks things.

[vjsamuel]

@ruflin should ID() be removed in the same PR or a subsequent one?

[ruflin]

In case ID() is not used anywhere else it would be nice to directly remove it here to make sure we don't forget it. Could you rebase your PR on master because there was a build issue earlier today which is fixed now.

[vjsamuel]

@ruflin i will file a subsequent PR immediately later in the day to remove ID. i have to fix collectbeat as well. if you dont mind, i will create an issue and track it against me so that it is definitely taken care.

[ruflin]

WFG

[exekias]

ok to test

[andrewkroh]

@ruflin @exekias Do we need to backport this to 5.6? Asking b/c of https://discuss.elastic.co/t/metricbeat-mongodb-leaks-connections-when-metricbeat-config-modules-reload-enabled-set-to-true/108723?u=andrewkroh

[ruflin]

@andrewkroh I think we could backport it, but as reloading was beta in 5.6 and is that 6.0 is out with the fix, I would rather encourage people to upgrade.