You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "itschrispeck (via GitHub)" <gi...@apache.org> on 2023/09/12 05:29:54 UTC
[GitHub] [pinot] itschrispeck commented on a diff in pull request #11558: Handle interrupt gracefully for Lucene mutable index
itschrispeck commented on code in PR #11558:
URL: https://github.com/apache/pinot/pull/11558#discussion_r1322407044
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/RealtimeLuceneTextIndex.java:
##########
@@ -109,27 +112,46 @@ public ImmutableRoaringBitmap getDictIds(String searchQuery) {
@Override
public MutableRoaringBitmap getDocIds(String searchQuery) {
MutableRoaringBitmap docIDs = new MutableRoaringBitmap();
- Collector docIDCollector = new RealtimeLuceneDocIdCollector(docIDs);
- IndexSearcher indexSearcher = null;
- try {
- Query query = new QueryParser(_column, _analyzer).parse(searchQuery);
- indexSearcher = _searcherManager.acquire();
- indexSearcher.search(query, docIDCollector);
- return getPinotDocIds(indexSearcher, docIDs);
- } catch (Exception e) {
- LOGGER
- .error("Failed while searching the realtime text index for column {}, search query {}, exception {}", _column,
- searchQuery, e.getMessage());
- throw new RuntimeException(e);
- } finally {
+ RealtimeLuceneDocIdCollector docIDCollector = new RealtimeLuceneDocIdCollector(docIDs);
+ // A thread interrupt during indexSearcher.search() can break the underlying FSDirectory used by the IndexWriter
+ // which the SearcherManager is created with. To ensure the index is never corrupted the search is executed
+ // in a child thread and the interrupt is handled in the current thread by canceling the search gracefully
+ Callable<MutableRoaringBitmap> searchCallable = () -> {
+ IndexSearcher indexSearcher = null;
try {
- if (indexSearcher != null) {
- _searcherManager.release(indexSearcher);
+ Query query = new QueryParser(_column, _analyzer).parse(searchQuery);
+ indexSearcher = _searcherManager.acquire();
+ indexSearcher.search(query, docIDCollector);
+ return getPinotDocIds(indexSearcher, docIDs);
+ } finally {
+ try {
+ if (indexSearcher != null) {
+ _searcherManager.release(indexSearcher);
+ }
+ } catch (Exception e) {
+ LOGGER.error(
+ "Failed while releasing the searcher manager for realtime text index for column {}, exception {}",
+ _column, e.getMessage());
+ }
+ }
+ };
+ FutureTask<MutableRoaringBitmap> searchTask = new FutureTask<>(searchCallable);
+ Thread searcherThread = new Thread(searchTask);
+ searcherThread.start();
+ try {
+ while (!searchTask.isDone()) {
Review Comment:
ahh, I don't think I can use `.notify()` in the finally block since it's likely to get called before the `.wait()` is even reached - and moving the `.wait()` into a loop as required ends up in about the same place we started
But I can simplify it with just `future.get()` and catch the interrupt, so thank you 😄
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/RealtimeLuceneTextIndex.java:
##########
@@ -109,27 +112,46 @@ public ImmutableRoaringBitmap getDictIds(String searchQuery) {
@Override
public MutableRoaringBitmap getDocIds(String searchQuery) {
MutableRoaringBitmap docIDs = new MutableRoaringBitmap();
- Collector docIDCollector = new RealtimeLuceneDocIdCollector(docIDs);
- IndexSearcher indexSearcher = null;
- try {
- Query query = new QueryParser(_column, _analyzer).parse(searchQuery);
- indexSearcher = _searcherManager.acquire();
- indexSearcher.search(query, docIDCollector);
- return getPinotDocIds(indexSearcher, docIDs);
- } catch (Exception e) {
- LOGGER
- .error("Failed while searching the realtime text index for column {}, search query {}, exception {}", _column,
- searchQuery, e.getMessage());
- throw new RuntimeException(e);
- } finally {
+ RealtimeLuceneDocIdCollector docIDCollector = new RealtimeLuceneDocIdCollector(docIDs);
+ // A thread interrupt during indexSearcher.search() can break the underlying FSDirectory used by the IndexWriter
+ // which the SearcherManager is created with. To ensure the index is never corrupted the search is executed
+ // in a child thread and the interrupt is handled in the current thread by canceling the search gracefully
+ Callable<MutableRoaringBitmap> searchCallable = () -> {
+ IndexSearcher indexSearcher = null;
try {
- if (indexSearcher != null) {
- _searcherManager.release(indexSearcher);
+ Query query = new QueryParser(_column, _analyzer).parse(searchQuery);
+ indexSearcher = _searcherManager.acquire();
+ indexSearcher.search(query, docIDCollector);
+ return getPinotDocIds(indexSearcher, docIDs);
+ } finally {
+ try {
+ if (indexSearcher != null) {
+ _searcherManager.release(indexSearcher);
+ }
+ } catch (Exception e) {
+ LOGGER.error(
+ "Failed while releasing the searcher manager for realtime text index for column {}, exception {}",
+ _column, e.getMessage());
+ }
+ }
+ };
+ FutureTask<MutableRoaringBitmap> searchTask = new FutureTask<>(searchCallable);
+ Thread searcherThread = new Thread(searchTask);
+ searcherThread.start();
+ try {
+ while (!searchTask.isDone()) {
Review Comment:
ahh, I don't think I can use `.notify()` in the finally block since it's likely to get called before the `.wait()` is even reached - and moving the `.wait()` into a loop as required ends up in about the same place we started
But I can simplify it with just `future.get()` and catch the interrupt, so 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org