after the lucene 5.3 upgrade, i looked at how ES uses lucene's filesystem locking. most places are ok, obtaining a lock and doing stuff in a try/finally. However NodeEnvironment is a totally different story. Can we fix the use of locking here?
deleteShardDirectorySafe is anything but safe. it calls deleteShardDirectoryUnderLock which doesn't actually delete under a lock either!!!! It calls this bogus method: acquireFSLockForPaths which acquires then releases locks. Why? Why? Why?
assertEnvIsLocked is only called under assert. why? Look at findAllIndices, its about to do something really expensive, why can't the call to ensureValid be a real check?
assertEnvIsLocked has a bunch of leniency, why in the hell would it return true when closed or when there are no locks at all, thats broken.
After this stuff is fixed, any places here doing heavy operations (e.g. N filesystem operations) should seriously consider calling ensureValid on any locks that are supposed to be held. It means you do N+1 operations or whatever but man, if what we are doing is not important, then why are we using fs locks?
after the lucene 5.3 upgrade, i looked at how ES uses lucene's filesystem locking. most places are ok, obtaining a lock and doing stuff in a try/finally. However NodeEnvironment is a totally different story. Can we fix the use of locking here?
deleteShardDirectorySafeis anything but safe. it callsdeleteShardDirectoryUnderLockwhich doesn't actually delete under a lock either!!!! It calls this bogus method:acquireFSLockForPathswhich acquires then releases locks. Why? Why? Why?assertEnvIsLockedis only called under assert. why? Look atfindAllIndices, its about to do something really expensive, why can't the call toensureValidbe a real check?assertEnvIsLockedhas a bunch of leniency, why in the hell would it returntruewhen closed or when there are no locks at all, thats broken.After this stuff is fixed, any places here doing heavy operations (e.g. N filesystem operations) should seriously consider calling
ensureValidon any locks that are supposed to be held. It means you do N+1 operations or whatever but man, if what we are doing is not important, then why are we using fs locks?