You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "slow-J (via GitHub)" <gi...@apache.org> on 2023/12/04 17:44:46 UTC
[PR] CheckIndex - Removal of some dead code [lucene]
slow-J opened a new pull request, #12876:
URL: https://github.com/apache/lucene/pull/12876
When I was working on `CheckIndex` in PR: https://github.com/apache/lucene/pull/12797, Issue: https://github.com/apache/lucene/issues/11023, I came across some dead code that I wanted to clean up as a follow-up.
List of changes:
* Removed `testPostings` and `testTermVectors`, these methods with 2 params are never called in Lucene
* Collections.sort -> List.sort, Collections.sort simply delegates to List.sort anyways
* println(String.format) -> printf
* Removal of some unused params in private methods
* Adding more usage of existing `numDocs` variable
Please let me know if any of these changes are not needed or are Lucene coding conventions and I will change them back.
If approved in current form, `testPostings` and `testTermVectors` with 2 params will need to be `@Deprecated` in a 9.x version.
--
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
Re: [PR] CheckIndex - Removal of some dead code [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12876:
URL: https://github.com/apache/lucene/pull/12876#issuecomment-1845277796
> Thanks @slow-J -- this looks great. I'll merge.
Thanks @mikemccand for the review, merge and port to 9_x!
I will tackle the removal of some of the `@lucene.experimental` next.
--
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
Re: [PR] CheckIndex - Removal of some dead code [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12876:
URL: https://github.com/apache/lucene/pull/12876#discussion_r1418816397
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -97,11 +96,11 @@
*/
public final class CheckIndex implements Closeable {
+ private final Directory dir;
+ private final Lock writeLock;
Review Comment:
Ahh thanks for adding missing `final` tags too. I wish we had a static checker for this.
##########
lucene/core/src/java/org/apache/lucene/index/CheckIndex.java:
##########
@@ -3216,11 +3213,11 @@ public static Status.DocValuesStatus testDocValues(
}
@FunctionalInterface
- private static interface DocValuesIteratorSupplier {
Review Comment:
Heh, the other PR about this.
--
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
Re: [PR] CheckIndex - Removal of some dead code [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand merged PR #12876:
URL: https://github.com/apache/lucene/pull/12876
--
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
Re: [PR] CheckIndex - Removal of some dead code [lucene]
Posted by "slow-J (via GitHub)" <gi...@apache.org>.
slow-J commented on PR #12876:
URL: https://github.com/apache/lucene/pull/12876#issuecomment-1842557831
> I'm not sure about the removal of these two functions, they're quite nice, only taking a CodecReader and an InfoStream. Removing them is forcing users to call the more complex functions and understand what a level is, or what the boolean means. Other changes look good to me.
Thanks @jpountz, will add these functions back in then.
I was wondering about the use of `@lucene.experimental` throughout this class. It has been experimental for > 10 years. Will the entire class or some parts, ever be classified as non-experimental?
--
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
Re: [PR] CheckIndex - Removal of some dead code [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12876:
URL: https://github.com/apache/lucene/pull/12876#issuecomment-1845176680
> I was wondering about the use of `@lucene.experimental` throughout this class. It has been experimental for > 10 years. Will the entire class or some parts, ever be classified as non-experimental?
LOL 10 years of experimental API! I bet Lucene has even older experimental APIs :) Too bad we don't have an "experimental API age reporting tool"...
Yeah this is a great question, when to remove the experimental API tag. We have no well defined process -- it's just devs (like you, thanks!) noticing and raising the question on a case by case basis. I do think some of these tags in `CheckIndex.java` could be removed, e.g. on each of the `testXXX` methods? But likely we should leave it on each of the `XXXStatus` returned classes, so we are free to break/change what metadata these classes return? And let's do it separately -- this PR looks like a great cleanup already. Progress not perfection!
--
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