thecoop commented on code in PR #15134:
URL: https://github.com/apache/lucene/pull/15134#discussion_r2387160473
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/analyzing/FreeTextSuggester.java:
##########
@@ -286,73 +282,67 @@ public void build(InputIterator iterator, double
ramBufferSizeMB) throws IOExcep
writer.addDocument(doc);
newCount++;
}
- reader = DirectoryReader.open(writer);
+ try (IndexReader reader = DirectoryReader.open(writer)) {
- Terms terms = MultiTerms.getTerms(reader, "body");
- if (terms == null) {
- throw new IllegalArgumentException("need at least one suggestion");
- }
+ Terms terms = MultiTerms.getTerms(reader, "body");
+ if (terms == null) {
+ throw new IllegalArgumentException("need at least one suggestion");
+ }
- // Move all ngrams into an FST:
- TermsEnum termsEnum = terms.iterator();
+ // Move all ngrams into an FST:
+ TermsEnum termsEnum = terms.iterator();
- Outputs<Long> outputs = PositiveIntOutputs.getSingleton();
- FSTCompiler<Long> fstCompiler =
- new FSTCompiler.Builder<>(FST.INPUT_TYPE.BYTE1, outputs).build();
+ Outputs<Long> outputs = PositiveIntOutputs.getSingleton();
+ FSTCompiler<Long> fstCompiler =
+ new FSTCompiler.Builder<>(FST.INPUT_TYPE.BYTE1, outputs).build();
- IntsRefBuilder scratchInts = new IntsRefBuilder();
- while (true) {
- BytesRef term = termsEnum.next();
- if (term == null) {
- break;
- }
- int ngramCount = countGrams(term);
- if (ngramCount > grams) {
- throw new IllegalArgumentException(
- "tokens must not contain separator byte; got token="
- + term
- + " but gramCount="
- + ngramCount
- + ", which is greater than expected max ngram size="
- + grams);
+ IntsRefBuilder scratchInts = new IntsRefBuilder();
+ while (true) {
+ BytesRef term = termsEnum.next();
+ if (term == null) {
+ break;
+ }
+ int ngramCount = countGrams(term);
+ if (ngramCount > grams) {
+ throw new IllegalArgumentException(
+ "tokens must not contain separator byte; got token="
+ + term
+ + " but gramCount="
+ + ngramCount
+ + ", which is greater than expected max ngram size="
+ + grams);
+ }
+ if (ngramCount == 1) {
+ totTokens += termsEnum.totalTermFreq();
+ }
+
+ fstCompiler.add(
+ Util.toIntsRef(term, scratchInts),
encodeWeight(termsEnum.totalTermFreq()));
}
- if (ngramCount == 1) {
- totTokens += termsEnum.totalTermFreq();
+
+ final FST<Long> newFst =
+ FST.fromFSTReader(fstCompiler.compile(),
fstCompiler.getFSTReader());
+ if (newFst == null) {
+ throw new IllegalArgumentException("need at least one suggestion");
}
+ fst = newFst;
+ count = newCount;
- fstCompiler.add(Util.toIntsRef(term, scratchInts),
encodeWeight(termsEnum.totalTermFreq()));
- }
+ // System.out.println("FST: " + fst.getNodeCount() + " nodes");
+
+ /*
+ PrintWriter pw = new PrintWriter("/x/tmp/out.dot");
+ Util.toDot(fst, pw, true, true);
+ pw.close();
+ */
- final FST<Long> newFst = FST.fromFSTReader(fstCompiler.compile(),
fstCompiler.getFSTReader());
- if (newFst == null) {
- throw new IllegalArgumentException("need at least one suggestion");
+ // Writer was only temporary, to count up bigrams,
+ // which we transferred to the FST, so now we
+ // rollback:
+ writer.rollback();
}
- fst = newFst;
- count = newCount;
-
- // System.out.println("FST: " + fst.getNodeCount() + " nodes");
-
- /*
- PrintWriter pw = new PrintWriter("/x/tmp/out.dot");
- Util.toDot(fst, pw, true, true);
- pw.close();
- */
-
- // Writer was only temporary, to count up bigrams,
- // which we transferred to the FST, so now we
- // rollback:
- writer.rollback();
- success = true;
} finally {
- try {
- if (success) {
- IOUtils.close(reader, dir);
Review Comment:
It helps to note here that `reader` and `dir` are closed whatever the
outcome. This means that both those can be moved to try-with-resources when
they are created, so they are always closed.
Previously, `writer` was only closed explicitly if an exception was thrown.
If it completed normally, only `rollback()` was called. But `IndexWriter`
checks for `close` being called after `rollback`, and makes sure that it is
only closed once. So this means that `writer` can also be in a
try-with-resources when it is created, as the extra implicit `close` at the end
of the outer try is a no-op if `rollback` is called, and closes it if an
exception is thrown (which is what the previous code also did).
In terms of live-ness, the try for the finally block the `IOUtils.rm` runs
in has increased in scope to include creating the `Document`, `FieldType`, and
`IndexWriter` at the start. Previously, if `IndexWriter` threw an exception,
the directory wouldn't be cleaned up. This is now the case, so it won't leak
`tempIndexPath` if `IndexWriter` throws an exception
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
