CURATOR-228 - Modified the background callback to explicitly handle t…

[cammckenzie]

…he NOAUTH case. This will now log a warning and set a flag indicating that an auth failure has occured.

[madrob]

Assuming all the tests pass, +1

[cammckenzie]

They all pass on my machine, so I guess they're all good.

[Randgalt]

Isn't logging enough? If we're going to have a special method to get auth issues here, we should do it for every recipe which seems like overkill to me. It's really a client error and not a bug in Curator. Logging only seems the correct approach.

[cammckenzie]

I originally just had logging but then there was no clean way to test it, thus the AtomicBoolean flag. I can make that package protected and @VisibileForTesting if you like.

[Randgalt]

That would be my vote. Otherwise we'd be introducing an API contract that would make its way to the other recipes.

[cammckenzie]

Done

[Randgalt]

Sorry to be pedantic. But, isn't this going to be the same issue for InterProcessMutex, etc.? i.e. any ZK recipe that creates nodes - which is a lot of them. I think it's a mistake to have specific handling like this in a single recipe.

[cammckenzie]

Possibly, we probably need to do an audit. I had a look at the InterProcessMutex and I think that it's ok because the node creation happens in the foreground. The PersistentEphemeralNode is problematic because it's all done in the background.

[Randgalt]

It would be better to just log the bad result code. Is there a reason that the check for NOAUTH is needed? Why not just log all non-expected result codes?

[cammckenzie]

Maybe we should just log an error and stop processing for any code that is not in RetryLoop.shouldRetry()? I was just trying to minimise the impact of the change as it was previously just handling the NodeExists and OK cases and retrying everything else.

[cammckenzie]

I had a look into implementing a Watch in the case where we get a NOAUTH error when trying to create the ephemeral node. The plan was that when the parent node ACL was modified that the watch would fire and we would retry. Unfortunately it seems that ACL changes in ZK don't fire Watches (I wasn't aware of this), so there doesn't seem to be a way to reliably know when the ACL changes without doing some sort of polling which is pretty ugly.