1) On WriteState variable privacy: 6 of one, half-dozen of the other. I made sure the WriteState variable was package private. I was looking at possibly some more unit tests dealing with our write state, so I didn't want to write a bunch of accessors just to deal with unit tests. In the unit test case, we don't really need to worry about synchronization either. My thought was to add accessor methods if we're going to use it outside of a unit test. Okay?
2) The lack of unlock() actually could have caused some extremely-rare deadlock conditions but only on exit, so no one's probably run across it. Just mainly wanted to fix poor practice.
Your thought is correct. However, I do need to make a small change that I had done internally, but lost when I refactored. This works because of some subtle interactions between server.stopRequested(), CompactSplitThread.lock, & HRegion.writeState.writesEnabled. States that can happen:
1) We get the lock & interrupt compactionQueue.poll(). It throws an InterruptedException, which calls continue, which fails the next while() check, which finishes the close
2) We get the lock & interrupt, but the thread is somewhere between the poll() and the lock(). [In new patch] CompactSplitThread.run() queries stopRequested() immediately after getting the lock(), which skips the compact/split code to return to the while() check and ...
3) We don't get the lock. HRegionServer.run() calls closeAllRegions(), which calls HRegion.close(), which sets the writeState. The compaction sees this, throws an InterruptedIOE, which is aborts the current compaction, goes to the while() check in CompactSplitThread.run() and ...