ISSUE 568 : Synchronize Kerberos ticket renewal for non-renewable ticket

[raju-saravanan]

Fixes #568

[guruchai]

I dont see any point having relogin and synchronizeReLogin. Please rename this as relogin and delete the other.

[raju-saravanan]

The code in try {} block and in else {} block have same lines of code which has been abstracted out as reLogin(). Its better to keep this abstracted out, as synchronizedReLogin() is wrapper over reLogin() which hold the actual logic to reLogin() while synchornizedReLogin() simply provides a synchronization on top of it.

[guruchai]

KERBEROS_SYNCHRONIZATION_TIMEOUT_MS does not belong here. And sharing the lock does not appear so elegant. How about this

1. Create a ReentrantReadWriteLock in KerberosLogin.
2. Use writelock in relogin method.
3. Push the synchronizedDoAction to KerberosLogin and use readlock for that.

You will limit the moving parts of KerberosLogin class.

[raju-saravanan]

@guruchai: I have exposed NOOPLogin class which will abstract out the login
() / doAction() mechanism in the schema registry.

[guruchai]

LGTM...

food for thought: Can we do away with all the additional checks for locks in KerberosLogin? relogin ideally should be an atomic operation. Almost all the KerberosLogin impls have their relogin wrapped inside a lock.
https://github.com/hortonworks/kafka/blob/bd037de41b621a6984a0577ab59e13c27c225931/clients/src/main/java/org/apache/kafka/common/security/kerberos/KerberosLogin.java#L359
https://github.com/apache/zookeeper/blob/d84b7a682eb4601005db9fac886079b1a9a4d4b0/zookeeper-server/src/main/java/org/apache/zookeeper/Login.java#L409

Similarly, on read lock side, no implementation would want to do an action without lock. So lets make that explicit by creating lock always. This simplifies the code and removes repititive checks. We can assume an indefinite timeout in default case.

[raju-saravanan]

We can't have reLogin() wrapped in a synchronized block as you have referenced in the previous comment. In the schema registry, we don't have a connection pool, we open up a new connection for every request made, so if we open a new connection between logout() and login() of reLogin() the server will throw 401. Zookeeper and Kafka the connection is reused after the request is over and hence it making a call between logout() and login() works as it still the cookie from the previous call.
In NOOPLogin where we want to use a plain connection to the server, we don't want a lock for every doAction() call. That is why I have not abstracted out the locking mechanism to its abstract class.

[guruchai]

@raju-saravanan
We can't have reLogin() wrapped in a synchronized block as you have referenced in the previous comment. In the schema registry, we don't have a connection pool, we open up a new connection for every request made, so if we open a new connection between logout() and login() of reLogin() the server will throw 401.

I am asking you to have only the synrchronizedRelogin method instead of both relogin and synrchronizedRelogin methods. You may use the WriteLock. synchronized as you said does not make sense. To me, relogin should be treated like an atomic operation.

Zookeeper and Kafka the connection is reused after the request is over and hence it making a call between logout() and login() works as it still the cookie from the previous call.

Connection reuse aside. The point is that we will not be able to make calls between logout and login, for any time, irrespective of presence of cookie or not.

[guruchai]

Thanks for the PR!