Upgrade to current Lucene 5.0.0 snapshot #8588
Conversation
| @@ -82,10 +84,10 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce | |||
| if (Files.exists(dir) == false) { | |||
| Files.createDirectories(dir); | |||
| } | |||
| logger.trace("obtaining node lock on {} ...", dir.toAbsolutePath()); | |||
| Directory luceneDir = FSDirectory.open(dir); | |||
| try { | |||
There was a problem hiding this comment.
can we use try / with here?
|
left once comment - LGTM otherwise |
|
looks good to me. Maybe @uschindler is curious about lockfactory changes. |
| NativeFSLockFactory lockFactory = new NativeFSLockFactory(dir); | ||
| Lock tmpLock = lockFactory.makeLock("node.lock"); | ||
| logger.trace("obtaining node lock on {} ...", dir.toAbsolutePath()); | ||
| Lock tmpLock = NativeFSLockFactory.INSTANCE.makeLock(luceneDir, "node.lock"); |
There was a problem hiding this comment.
this should be: Lock tmpLock = luceneDir.makeLock("node.lock");
(using LockFactories directly is no longer wanted, they are no first class citizen anymore - the actual locking is responsibility of the Directory impl, BaseDirectory in Lucene just delegates to a lock factory, but thats an impl detail).
And if you want NativeFSLockFactory explicit, pass it to the FSDirectory ctor/factory.
|
Thank you for all the feedback, I folded it all in @uschindler |
| public synchronized String getLockID() { | ||
| return distributor.primary().getLockID(); | ||
| /** Called by makeLock's wrapped lock, to record the lock file. */ | ||
| synchronized void addNameDirMapping(String name, Directory dir) { |
There was a problem hiding this comment.
this method can be private now
|
See my comments above. Otherwise its correct API-wise. It also took me a long time while refactoring LockFactory in Lucene that you don't need one in most cases. LockFactory is just an implementation detail of BaseDirectory which should only be used for actual "real" directory implementations. Everything that wraps or filters should use Directory (for complex delegation like FileSwitchDirectory) or FilterDirectory (for simple delagation). |
|
I pushed another commit w/ feedback from @uschindler (thanks Uwe!); I think it's ready. |
|
LGTM |
|
+1 |
It's dangerous to release this lock because it means another ES somehow still has an IndexWriter open on the directory, which can lead to corruption. Instead, we don't release the lock, and the user will see an exception on init. This was already fixed on master with #8588; I'm backporting to 1.x here. Closes #8892
It's dangerous to release this lock because it means another ES somehow still has an IndexWriter open on the directory, which can lead to corruption. Instead, we don't release the lock, and the user will see an exception on init. This was already fixed on master with #8588; I'm backporting to 1.x here. Closes #8892
It's dangerous to release this lock because it means another ES somehow still has an IndexWriter open on the directory, which can lead to corruption. Instead, we don't release the lock, and the user will see an exception on init. This was already fixed on master with #8588; I'm backporting to 1.x here. Closes #8892
clintongormley
changed the title
It's dangerous to release this lock because it means another ES somehow still has an IndexWriter open on the directory, which can lead to corruption. Instead, we don't release the lock, and the user will see an exception on init. This was already fixed on master with elastic#8588; I'm backporting to 1.x here. Closes elastic#8892
It's dangerous to release this lock because it means another ES somehow still has an IndexWriter open on the directory, which can lead to corruption. Instead, we don't release the lock, and the user will see an exception on init. This was already fixed on master with elastic#8588; I'm backporting to 1.x here. Closes elastic#8892
clintongormley
added
:Core/Infra/Core
A few recent lucene changes to fold in:
We've removed IndexWriter.unLock in Lucene, so I also removed "clear
lock on startup" in Elasticsearch. A couple test cases were missing
engine.close() before opening a new engine.
I exposed the new SerbianNormalizationFilterFactory.
I noticed that we silently use NoLockFactory if you have a typo in
your index.store.fs.fs_lock; I changed this to throw
StoreException instead. I think it's scary to be lenient
here?
The trickiest change was DistributorDirectory, and how it tracks the
file created by the lock factory. For the writeFiles boolean, I had
to remove the check that "lockFactory instanceof FSLockFactory"; maybe
we could use reflection to get this, if it's really necessary? (Do we
really support using NoLockFactory w/ FSDirectory?). Or maybe instead
we could check if the class name of the lock/factory is NoLock, but
MockDirWrapper wraps in AssertingLock...
I added some harmless "synchronized" to methods (all callers of these
private methods are already sync'd) just to make it clear on quick
look that all access/changes to nameDirMapping are in fact sync'd.
I also fixed ConcurrentMergeSchedulerProvider to disable Lucene's CMS
stalling (override w/ an empty maybeStall method). We can simplify
things here: remove EnableMergeScheduler, the separate MERGE thread
pool, let IndexWriter launch merges when they are needed (not
separately, once every second), etc., since Lucene won't stall
indexing threads anymore ... I'll open a separate issue for this.