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 2020/08/10 18:42:28 UTC
[GitHub] [lucene-solr] madrob opened a new pull request #1732: Clean up many small fixes
madrob opened a new pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732
* Abstract classes don't need public constructors since they can only be
called by subclasses
* Don't escape html characters in @code tags in javadoc
* Fixed a few int/long arithmetic
* Use Arrays.toString instead of implicit byte[].toString
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468227099
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java
##########
@@ -324,12 +324,12 @@ synchronized void doOnAbort(DocumentsWriterPerThread perThread) {
}
}
- private void checkoutAndBlock(DocumentsWriterPerThread perThread) {
+ private synchronized void checkoutAndBlock(DocumentsWriterPerThread perThread) {
Review comment:
https://issues.apache.org/jira/browse/LUCENE-9453 I explain in that issue why I believe it is minor, but it will help to get more eyes on it
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468221630
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocValuesUpdate.java
##########
@@ -152,12 +152,12 @@ static BytesRef readFrom(DataInput in, BytesRef scratch) throws IOException {
}
NumericDocValuesUpdate(Term term, String field, Long value) {
- this(term, field, value != null ? value.longValue() : -1, BufferedUpdates.MAX_INT, value != null);
+ this(term, field, value != null ? value : -1, BufferedUpdates.MAX_INT, value != null);
}
- private NumericDocValuesUpdate(Term term, String field, long value, int docIDUpTo, boolean hasValue) {
Review comment:
There were 16 instances of `Upto` and 4 of `UpTo` so I went with the more common one for consistency. Happy to switch the other way if it's more correct according to English. Looking it up now and looks like "upto" isn't a word?
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java
##########
@@ -324,12 +324,12 @@ synchronized void doOnAbort(DocumentsWriterPerThread perThread) {
}
}
- private void checkoutAndBlock(DocumentsWriterPerThread perThread) {
+ private synchronized void checkoutAndBlock(DocumentsWriterPerThread perThread) {
Review comment:
I'll split this out.
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468631394
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -440,7 +440,7 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (format >= VERSION_70) { // oldest supported version
CodecUtil.checkFooter(input, priorE);
} else {
- throw IOUtils.rethrowAlways(priorE);
Review comment:
The original compiler complaint was that the throw is inside the finally block. Could I replace the "Unreachable code" at the end with this rethrow? I believe the logic will be the same.
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r473360728
##########
File path: lucene/sandbox/src/java/org/apache/lucene/codecs/idversion/VersionBlockTreeTermsWriter.java
##########
@@ -582,7 +583,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
assert ent.isTerm: "i=" + i;
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + Arrays.toString(term.termBytes) + " prefix=" + prefix;
Review comment:
yikes, thank you.
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468220535
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -94,7 +94,7 @@
* Create a new Analyzer, reusing the same set of components per-thread
* across calls to {@link #tokenStream(String, Reader)}.
*/
- public Analyzer() {
Review comment:
I understand that it's notionally an API change, but `abstract` classes have no reason for public constructors. We can make everything protected and the subclasses that people use will be able to pick it up. I was over-zealous in a couple places going to package instead of protected, I'll fix that up.
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468367148
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java
##########
@@ -709,7 +709,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + new String(term.termBytes) + " prefix=" + prefix;
Review comment:
They should be. You can also dump it as a byte array for consistency with other changes you made.
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468622877
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
Review comment:
I think it's because the field is final and there is a getter for it, so the code analyzer prefers encapsulation?
----------------------------------------------------------------
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.
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-solr] madrob merged pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r473394137
##########
File path: lucene/test-framework/src/java/org/apache/lucene/index/BaseTermVectorsFormatTestCase.java
##########
@@ -504,8 +504,8 @@ protected void assertEquals(RandomTokenStream tk, FieldType ft, Terms terms) thr
}
try {
Review comment:
So it turns out be be less clean, since two of the implementations throw IllegalStateException and two throw AssertionError.
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r469027938
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
Review comment:
Maybe. It doesn't matter though - this changes the API of a class that's been there for ages. I bet there is a class out there somewhere (let's say A extends Analyzer) and another one (B extends A) where A overrides the getter but B reaches out for the original field. Do we want this to break just to hide a field that can be useful for subclasses just to silence an automatic code inspection? I don't think we should.
----------------------------------------------------------------
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.
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-solr] madrob commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468220684
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java
##########
@@ -709,7 +709,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + new String(term.termBytes) + " prefix=" + prefix;
Review comment:
Are these UTF-8? I wasn't sure, and hoped somebody would let me know during review.
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r471075532
##########
File path: lucene/sandbox/src/java/org/apache/lucene/codecs/idversion/VersionBlockTreeTermsWriter.java
##########
@@ -610,7 +611,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingEntry ent = pending.get(i);
if (ent.isTerm) {
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + Arrays.toString(term.termBytes) + " prefix=" + prefix;
Review comment:
term.toString() again?
##########
File path: lucene/test-framework/src/java/org/apache/lucene/index/BaseTermVectorsFormatTestCase.java
##########
@@ -504,8 +504,8 @@ protected void assertEquals(RandomTokenStream tk, FieldType ft, Terms terms) thr
}
try {
Review comment:
Since you're cleaning up I think it'd be better to use assertThrows() with lambda...
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocumentsWriterDeleteQueue.java
##########
@@ -510,8 +510,8 @@ public String toString() {
}
private boolean forceApplyGlobalSlice() {
Review comment:
I didn't look at the code but this looks suspicious. The reordering here changes happens-before relationship between these statements. Please leave the order of assignment of tail as it was (inside the locked block).
##########
File path: lucene/sandbox/src/java/org/apache/lucene/codecs/idversion/VersionBlockTreeTermsWriter.java
##########
@@ -582,7 +583,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
assert ent.isTerm: "i=" + i;
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + Arrays.toString(term.termBytes) + " prefix=" + prefix;
Review comment:
inconsistent with other calls (should use term.toString())?
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r473913739
##########
File path: lucene/test-framework/src/java/org/apache/lucene/index/BaseTermVectorsFormatTestCase.java
##########
@@ -504,8 +504,8 @@ protected void assertEquals(RandomTokenStream tk, FieldType ft, Terms terms) thr
}
try {
Review comment:
@jpountz may be interested in why this is inconsistent between implementations?
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468106643
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java
##########
@@ -709,7 +709,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
+ assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + new String(term.termBytes) + " prefix=" + prefix;
Review comment:
This is wrong, uses default locale.
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -94,7 +94,7 @@
* Create a new Analyzer, reusing the same set of components per-thread
* across calls to {@link #tokenStream(String, Reader)}.
*/
- public Analyzer() {
Review comment:
Can you not change those scopes in public API classes? This applies here and in other places -- protected changed to package-scope for source is not really an API-compatible change.
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java
##########
@@ -324,12 +324,12 @@ synchronized void doOnAbort(DocumentsWriterPerThread perThread) {
}
}
- private void checkoutAndBlock(DocumentsWriterPerThread perThread) {
+ private synchronized void checkoutAndBlock(DocumentsWriterPerThread perThread) {
Review comment:
These are serious changes... you're adding synchronization on core classes. I don't think they should be piggybacked on top of trivial ones - I'm sure @s1monw would chip in whether this synchronization here makes sense but he'll probably overlook if it's a bulk of trivial changes on top.
##########
File path: lucene/core/src/java/org/apache/lucene/index/DocValuesUpdate.java
##########
@@ -152,12 +152,12 @@ static BytesRef readFrom(DataInput in, BytesRef scratch) throws IOException {
}
NumericDocValuesUpdate(Term term, String field, Long value) {
- this(term, field, value != null ? value.longValue() : -1, BufferedUpdates.MAX_INT, value != null);
+ this(term, field, value != null ? value : -1, BufferedUpdates.MAX_INT, value != null);
}
- private NumericDocValuesUpdate(Term term, String field, long value, int docIDUpTo, boolean hasValue) {
Review comment:
previous version was correct camel case (upTo).
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468366809
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -94,7 +94,7 @@
* Create a new Analyzer, reusing the same set of components per-thread
* across calls to {@link #tokenStream(String, Reader)}.
*/
- public Analyzer() {
Review comment:
The constructors I understand, fine (although it's really a no-op change, as you indicated).
----------------------------------------------------------------
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.
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-solr] uschindler commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468374779
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
Review comment:
Removing this would break many outside analyzers. I know it's seldom that analyzers access these fields, but this is a real backward breaking change. Don't do this.
I have no problem with the ctors, but here it's serious! Why did Intellij suggest a change like that? Looks like it was not so intelligent. 🤨
----------------------------------------------------------------
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.
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-solr] madrob commented on pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#issuecomment-688947909
@vthacker this is also some error-prone warnings, so may overlap with what you're looking at.
----------------------------------------------------------------
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.
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-solr] uschindler commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468376068
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
Review comment:
There's also no risk somebody could do anything wrong. It's both (also next one) final field and the it's for consuming only.
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r468369400
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -440,7 +440,7 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (format >= VERSION_70) { // oldest supported version
CodecUtil.checkFooter(input, priorE);
} else {
- throw IOUtils.rethrowAlways(priorE);
Review comment:
Leave this as it was (with throw ...) - don't know whether IntelliJ is smart enough to detect this method always throws an exception but other compilers are not (and this ensures the compiler sees it as a the only codepath leaving the clause).
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java
##########
@@ -709,7 +710,8 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
Review comment:
that 'term' class (PendingTerm) actually has a perfectly fine toString method... why not just remove termBytes and let it do its job?
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/blocktree/BlockTreeTermsWriter.java
##########
@@ -741,7 +743,8 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
if (ent.isTerm) {
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
Review comment:
Same here.
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
Review comment:
Don't change to package private scope here. It will prevent subclasses from outside of the package from accessing those fields (and there may be classes outside of Lucene code doing that).
##########
File path: lucene/core/src/java/org/apache/lucene/analysis/Analyzer.java
##########
@@ -367,12 +367,12 @@ public void close() {
/**
* Original source of the tokens.
*/
- protected final Consumer<Reader> source;
+ final Consumer<Reader> source;
/**
* Sink tokenstream, such as the outer tokenfilter decorating
* the chain. This can be the source if there are no filters.
*/
- protected final TokenStream sink;
Review comment:
Same here.
##########
File path: lucene/codecs/src/java/org/apache/lucene/codecs/blocktreeords/OrdsBlockTreeTermsWriter.java
##########
@@ -604,7 +605,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
assert ent.isTerm: "i=" + i;
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
Review comment:
term has a toString method - use it.
##########
File path: lucene/codecs/src/java/org/apache/lucene/codecs/blocktreeords/OrdsBlockTreeTermsWriter.java
##########
@@ -640,7 +641,7 @@ private PendingBlock writeBlock(int prefixLength, boolean isFloor, int floorLead
PendingEntry ent = pending.get(i);
if (ent.isTerm) {
PendingTerm term = (PendingTerm) ent;
- assert StringHelper.startsWith(term.termBytes, prefix): "term.term=" + term.termBytes + " prefix=" + prefix;
Review comment:
term has a toString method - use it.
----------------------------------------------------------------
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.
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-solr] dweiss commented on a change in pull request #1732: Clean up many small fixes
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #1732:
URL: https://github.com/apache/lucene-solr/pull/1732#discussion_r469031590
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -440,7 +440,7 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (format >= VERSION_70) { // oldest supported version
CodecUtil.checkFooter(input, priorE);
} else {
- throw IOUtils.rethrowAlways(priorE);
Review comment:
And what's wrong about a throw from within finally? A finally block is technically just a block of code, like any other. The compiler very likely assumes you're suppressing an exception if you throw from within finally but it's not the case here.
I don't know if moving that throw will change the logic. Maybe not. Maybe yes. Given the two options, I wouldn't touch it. My concern was that you slipped such things as part of an otherwise "trivial" set of patches.
----------------------------------------------------------------
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.
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