You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/12/20 18:56:27 UTC
[GitHub] [lucene] uschindler opened a new pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
uschindler opened a new pull request #556:
URL: https://github.com/apache/lucene/pull/556
See https://issues.apache.org/jira/browse/LUCENE-10330
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #556:
URL: https://github.com/apache/lucene/pull/556#issuecomment-998261429
I improved the test a bit (it separately asserts before reaching out to MMapDirectory that the core moudle can read `jdk.unsupported`. This is there to verify that the code [here](https://github.com/apache/lucene/pull/556/files#diff-efeb36d227b58a0ad42ddf35d0e260ca8010021f8dbc78627333085defa16a13R374) actually works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772603688
##########
File path: lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
##########
@@ -36,7 +36,7 @@ protected Directory getDirectory(Path path, int maxChunkSize) throws IOException
@Override
public void setUp() throws Exception {
super.setUp();
- assumeTrue(MMapDirectory.UNMAP_NOT_SUPPORTED_REASON, MMapDirectory.UNMAP_SUPPORTED);
+ assertTrue(MMapDirectory.UNMAP_NOT_SUPPORTED_REASON, MMapDirectory.UNMAP_SUPPORTED);
Review comment:
possibly this check should be class-level, because method-level makes it fail hundreds of times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler edited a comment on pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler edited a comment on pull request #556:
URL: https://github.com/apache/lucene/pull/556#issuecomment-998261429
I improved the test a bit (it separately asserts before reaching out to MMapDirectory that the core module can read `jdk.unsupported`). This is there to verify that the code [here](https://github.com/apache/lucene/pull/556/files#diff-efeb36d227b58a0ad42ddf35d0e260ca8010021f8dbc78627333085defa16a13R374) actually works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772645222
##########
File path: lucene/distribution.tests/src/test/org/apache/lucene/distribution/TestModularLayer.java
##########
@@ -292,4 +293,17 @@ public void testAllOpenPackagesInSync() throws IOException {
.collect(Collectors.toCollection(TreeSet::new));
}
}
+
Review comment:
I created a PR for you, Uwe - seems to work for me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler edited a comment on pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler edited a comment on pull request #556:
URL: https://github.com/apache/lucene/pull/556#issuecomment-998261429
I improved the test a bit (it separately asserts before reaching out to MMapDirectory that the core moudle can read `jdk.unsupported`). This is there to verify that the code [here](https://github.com/apache/lucene/pull/556/files#diff-efeb36d227b58a0ad42ddf35d0e260ca8010021f8dbc78627333085defa16a13R374) actually works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772604117
##########
File path: lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
##########
@@ -36,7 +36,7 @@ protected Directory getDirectory(Path path, int maxChunkSize) throws IOException
@Override
public void setUp() throws Exception {
super.setUp();
- assumeTrue(MMapDirectory.UNMAP_NOT_SUPPORTED_REASON, MMapDirectory.UNMAP_SUPPORTED);
+ assertTrue(MMapDirectory.UNMAP_NOT_SUPPORTED_REASON, MMapDirectory.UNMAP_SUPPORTED);
Review comment:
So I'd change this to `@BeforeClass`. Any opinions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772628223
##########
File path: lucene/distribution.tests/src/test/org/apache/lucene/distribution/TestModularLayer.java
##########
@@ -292,4 +293,17 @@ public void testAllOpenPackagesInSync() throws IOException {
.collect(Collectors.toCollection(TreeSet::new));
}
}
+
Review comment:
I know, but I wasn't able to do this. Maybe because of the still open bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler merged pull request #556:
URL: https://github.com/apache/lucene/pull/556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772635579
##########
File path: lucene/distribution.tests/src/test/org/apache/lucene/distribution/TestModularLayer.java
##########
@@ -292,4 +293,17 @@ public void testAllOpenPackagesInSync() throws IOException {
.collect(Collectors.toCollection(TreeSet::new));
}
}
+
Review comment:
Let me see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a change in pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #556:
URL: https://github.com/apache/lucene/pull/556#discussion_r772625522
##########
File path: lucene/distribution.tests/src/test/org/apache/lucene/distribution/TestModularLayer.java
##########
@@ -292,4 +293,17 @@ public void testAllOpenPackagesInSync() throws IOException {
.collect(Collectors.toCollection(TreeSet::new));
}
}
+
Review comment:
I think this should go into modular tests for lucene.core(.test) instead of the distribution tests - there it makes more sense and you don't have to load the module dynamically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #556: LUCENE-10330: Make the mmap directory tests fail by default, if unmapping does not work
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #556:
URL: https://github.com/apache/lucene/pull/556#issuecomment-998256545
Thanks Dawid. I will add another check to the modular test, that asserts that unsupported module is readable.
I just want to assert that the code I added to the MMapDirectory message producer works well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org