You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by guidotag <gi...@git.apache.org> on 2015/02/03 02:56:04 UTC
[GitHub] lucenenet pull request: Minor fixes in store
GitHub user guidotag opened a pull request:
https://github.com/apache/lucenenet/pull/70
Minor fixes in store
See the comment on the penultimate commit.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/guidotag/lucenenet ms_store_2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucenenet/pull/70.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #70
----
commit 68ba6d299a8c1beeb8caec024b84b90397372620
Author: Guido Tagliavini Ponce <gu...@gmail.com>
Date: 2015-01-15T23:56:39Z
Merge pull request #1 from apache/master
Pulling recent changes.
commit e573fefcda401f15b8c5c9808a4fc4606567ec92
Author: Guido Tagliavini Ponce <gu...@gmail.com>
Date: 2015-01-21T19:51:18Z
Merge pull request #2 from apache/master
Pulling recent changes.
commit 9b17d76945d20b8769ee1a2c64e9ca22ab39465f
Author: Guido Tagliavini Ponce <t-...@microsoft.com>
Date: 2015-01-26T19:02:42Z
Merge branch 'master' of https://github.com/guidotag/lucenenet
commit 06c8bea20a8f91b6f78320895c669ac9af647b38
Author: Guido Tagliavini Ponce <t-...@microsoft.com>
Date: 2015-01-27T19:51:31Z
Merge remote-tracking branch 'apache/master'
commit ee54ef856ff1c4c50d05caab48b773482131a6ea
Author: Guido Tagliavini Ponce <t-...@microsoft.com>
Date: 2015-01-29T18:45:32Z
Merge branch 'master' of https://github.com/apache/lucenenet
commit 6137a84724a5424cb19aee261a6415a88fd839c2
Author: Guido Tagliavini Ponce <t-...@microsoft.com>
Date: 2015-02-03T01:48:17Z
Changed the GetCanonicalPath method. Now it does not create the file if it doesn't exist (matching the Java version).
Also changed a test that was failing because System.IO.Directory.Delete assumes the file exists. In Java, the deletion was done using a hand-made function that checked the file existance (and that's why the test passed).
commit 5a6e2cc9be96fdceda21fc14b924df4c9dff1576
Author: Guido Tagliavini Ponce <t-...@microsoft.com>
Date: 2015-02-03T01:52:07Z
Merge branch 'master' of https://github.com/apache/lucenenet into ms_store_2
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72628999
I don't mind Lucene.NET 4.8 being non-compatible with Java Lucene 4.8 in that regard since it doesn't affect anything but our "create new index" story and the exception we throw when the index doesn't exist, but I'd rather make sure first there isn't special reason for things being the (weird) way they are now
/cc @rmuir
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by uschindler <gi...@git.apache.org>.
Github user uschindler commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72822745
Hi,
I checked the Lucene Core 4.8 code. In fact it does not create the directory. The comment is just bogus. I removed it in Lucene 4.10: http://svn.apache.org/r1657033
In Lucene 5, the move to Java NIO.2 now creates the direczory in the ctor. This is in my opinion wrong to do, so I started a discussion. In fact, Mike McCandless stated, that there was already a bug open about this: https://issues.apache.org/jira/browse/LUCENE-1464
The thread is CC'ed to the dev@lucenenet list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by laimis <gi...@git.apache.org>.
Github user laimis commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72773139
@synhershko @guidotag I just setup Lucene 4.8 on my local with source code and IDE so that I can debug (some of those multithreading bugs are brutal to catch otherwise), and can confirm that in Java version, FSDirectory does not create the directory, it only says that it does in the comment. If you look at the code, it make sense:
https://github.com/apache/lucene-solr/blob/lucene_solr_4_8_0/lucene/core/src/java/org/apache/lucene/store/FSDirectory.java#L128
There is no explicit create being called, and constructing a File class with a path does not create a file that represents such a path. When the port was being done, whoever read that comment for "getCanonicalPath" that said "creating it if it doesn't exist." simply got mislead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72826736
ok @laimis @guidotag let's mimic the actual behavior and not the intended behavior. The tests should pass without changing them, and the exceptions thrown as meaningful as they can be. In fact, lets remove `GetCanonicalPath` altogether if possible.
Thanks @uschindler !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by guidotag <gi...@git.apache.org>.
Github user guidotag commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72915699
OK @laimis. TestDontCreate needs to be changed just a little bit to check that the directory exists (it actually won't) before trying to delete it. Besides, there seems to be no way of getting rid of GetCanonicalPath by using a DirectoryInfo method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by uschindler <gi...@git.apache.org>.
Github user uschindler commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-84018532
Thanks! Looks fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucenenet/pull/70
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by guidotag <gi...@git.apache.org>.
Github user guidotag commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72703560
@uschindler I didn't know what Lucene 5.0 does. I just tried to reproduce Lucene 4.8 behavior.
Essentially the only difference between TestDontCreate in Java and C# was that the first one didn't create the directory in the FS. That didn't make a difference in the final result. When I did the change in GetCanonicalPath, Lucene.Net's test started to fail because there was no directory to delete (Lucene does not fail because it checks if the directory exists before attempting the deletion).
Note that the tests TestNRTCachingDirectory/TestNoDir and TestFileSwitchDirectory/TestNoDir now pass thanks to this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72607213
This is very weird that the file isn't being created despite the call to `file.Create();`
I'd like to have @uschindler's eyes on this before pulling this in
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by uschindler <gi...@git.apache.org>.
Github user uschindler commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72623789
I still have no idea why this one test in Lucene seems to se a different behaviour. Maybe its some bug, I will look into it possibly a bit later. Just came back from FOSDEM have to catch up first.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by uschindler <gi...@git.apache.org>.
Github user uschindler commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72623626
@rcmuir 's patches for Java 7 changed that, as far as I remember. I will ask him. In fact if a directory does not exist FSDirectory in Lucene 5.0 will create it in its constructor. It calls Files.createDirectories(Path), which is "mkdir -p": http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createDirectories(java.nio.file.Path,%20java.nio.file.attribute.FileAttribute...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-73765620
I'm assigning this to myself. I'll work on this next week and get some well-defined behavior suitable for the CLR in place.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request: Minor fixes in store
Posted by uschindler <gi...@git.apache.org>.
Github user uschindler commented on the pull request:
https://github.com/apache/lucenenet/pull/70#issuecomment-72958293
Hi,
In Lucene Java the GetCanonicalPath is needed for the following: Java has the problem that creating a native lock only affects *other* processes from using the directory. It does not prevent the same process from writing to the same index directory. Because of that NativeFSLockFactory keeps a static set of Path names to also ensure locking in the same JVM/process. To make sure that this locking works we need the "canonical name" of the directory, otherwise it is not guaranteed that locking really works.
If this is not an issue on windows (if you lock a file, the file lock is visible to the same process, too), then you won't need that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---