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/10/06 21:06:38 UTC
[GitHub] [lucene] rmuir opened a new pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
rmuir opened a new pull request #360:
URL: https://github.com/apache/lucene/pull/360
BaseChunkedDirectoryTestCase is an extension of BaseDirectoryTestCase
where the concrete test class instantiates with a specified chunk size.
It then tries to test boundary conditions around all the chunking.
--
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 #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #360:
URL: https://github.com/apache/lucene/pull/360#discussion_r723900036
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
Just a note. I wonder why the restrictions in the public constructor are there - I can't remember the motivation for these. I'm guessing they're there because small-ish blocks don't make much sense for real-life applications in production and this guards against such insane input arguments. On the other hand, why not leave the decision to whoever is using the class (such as tests in this case. Can't make my mind whether the package-private constructor makes more sense than just opening up the range of arguments to a full spectrum of allowed values. You decide. :)
--
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] rmuir commented on pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #360:
URL: https://github.com/apache/lucene/pull/360#issuecomment-939291709
I removed the custom pkg-private ctor for testing. Added constants for min/max values. Added some javadocs for the ctors and some tests for the illegal values. `expectThrows` was mysteriously missing, it was because the test didnt extend LuceneTestCase, I addressed that too.
--
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] rmuir commented on a change in pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #360:
URL: https://github.com/apache/lucene/pull/360#discussion_r724042253
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
I had the same problem. I tend to lean towards the defaults should be fast, but the permissible values should be ones that work correctly.
It is also not efficient to use MMapDirectory with 4-byte memory mappings: but it works correctly and we take advantage of this in testing.
Funny history to this test in question: https://issues.apache.org/jira/browse/LUCENE-2627
--
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] rmuir merged pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
rmuir merged pull request #360:
URL: https://github.com/apache/lucene/pull/360
--
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] rmuir commented on a change in pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #360:
URL: https://github.com/apache/lucene/pull/360#discussion_r724042253
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
I had the same problem. I tend to lean towards the defaults should be fast, but the permissible values should be ones that work correctly.
It is also not efficient to use MMapDirectory with 4-byte memory mappings: but it works correctly and we take advantage of this in testing.
Funny history to this test in question: https://issues.apache.org/jira/browse/LUCENE-2627
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
I am working on an updated commit: will be later this evening. Trying to add some javadocs here anyway and stuff like that. I think in most cases users should use the default or simple ctors (estimated size) and not specify min/max block sizes directly anyway: we should label this one "expert". And reserve the right to change it in the future (for example, if we aligned our index files to at least 8 bytes, we could then make `1 << 3` the minimum size and simplify a ton of code: none of the primitive-type reads would ever span boundaries anymore.
--
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 #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #360:
URL: https://github.com/apache/lucene/pull/360#discussion_r723900036
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
Just a note. I wonder why the restrictions in the public constructor are there - I can't remember the motivation for these. I'm guessing they're there because small-ish blocks don't make much sense for real-life applications in production and this guards against such insane input arguments. On the other hand, why not leave the decision to whoever is using the class (such as tests in this case. Can't make my mind whether the package-private constructor makes more sense than just opening up the range of arguments to a full spectrum of allowed values. You decide. :)
--
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] rmuir commented on a change in pull request #360: LUCENE-10155: Refactor TestMultiMMap into a BaseChunkedDirectoryTestCase
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #360:
URL: https://github.com/apache/lucene/pull/360#discussion_r724261686
##########
File path: lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
##########
@@ -137,6 +137,14 @@ public ByteBuffersDataOutput(
this.blockReuse = Objects.requireNonNull(blockReuse, "Block reuse must not be null.");
}
Review comment:
I am working on an updated commit: will be later this evening. Trying to add some javadocs here anyway and stuff like that. I think in most cases users should use the default or simple ctors (estimated size) and not specify min/max block sizes directly anyway: we should label this one "expert". And reserve the right to change it in the future (for example, if we aligned our index files to at least 8 bytes, we could then make `1 << 3` the minimum size and simplify a ton of code: none of the primitive-type reads would ever span boundaries anymore.
--
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