Thanks Erik for the updates.
I'm not very familiar with ThreadLocal either. Thanks for the pointer and I agree it's a good idea to remove(). This way when the thread is recycled by the thread pool, the value will be reset.
One corner case is when readUnlock is called before any readLock. Will we see an NPE? Since we are not overriding ThreadLocal#initialValue.
5. This actually isn't possible under the current FSNamesystemLock design. Within FSNamesystemLock, the methods to actually lock are not exposed. It simply has methods writeLock and readLock which return the respective lock, then the calling class uses the lock/lockIinterruptibly methods on the returned objects.
Good thoughts. I was initially thinking about moving the time-tracking logic into FSNamesystemLock. E.g. we can move writeLockHeldTimeStamp and readLockHeldTimeStamp into FSNamesystemLock. The lock class can take care of the time-tracking logic. This way lock and lockIinterruptibly can share the logic and the time-tracking logic can be unit tested more easily. We can also consider consolidating the threshold and needReport logic there too. Maybe also add richer APIs to the FSNamesystemLock class like reporting the overall lock held time of a thread (instead of a single reentrant period). But I think we should discuss all these as a separate follow-on anyway. Also, since we are printing the stack trace, the methods which actually acquire the lock are still logged right?
The test code looks very good.
So +1 after we clarify the readUnlock NPE concern.