On Mon, Oct 31, 2011 at 03:00:26PM +0100, Michael Hanselmann wrote: > Hi Iustin > > Am 26. Oktober 2011 13:55 schrieb Iustin Pop <[email protected]>: > > I… somewhat dislike this change/fear it can lead to breakage. Currently, > > we pass a static list, whereas in the future we pass functions. > > > > Basically right now, the list of "frozen" before the upload starts. With > > this change, the node:ip mapping can change even after we start the > > call, and because we share objects without locks on the nodes > > themselves, we don't know if the objects (the nodes) themselves will > > stay consistent. > > I had another look. There is exactly one case where a node's primary > IP address can change once it's been added. That would be re-adding a > node with DNS resolving to a different IP address. There is indeed a > race condition, but it already existed with the static list passed to > the RPC layer. I hope one day we'll just pass around copies, not > references to these objects. > > That said, since it's no significant change, can this go in?
I'm not as worried about the safety of this patch right now, but of the fact that the code flow is hard to understand. I.e. you pass a custom resolver to the RpcResolver which calls back into the ConfigWriter itself *during* the time that ConfigWriter makes the RPC calls. This seems brittle, if not in the current patch, but for future changes. Can't we keep the 'build static resolver, pass it to rpcrunner' model? thanks, iustin
