NightOwl888 commented on PR #1080: URL: https://github.com/apache/lucenenet/pull/1080#issuecomment-2608162890
Repeating this comment from https://github.com/apache/lucenenet/pull/1119#discussion_r1925579476: After taking a step back from this and considering that we are working on support for `CancellationToken`s, I wonder if we should consider removing `LimitedConcurrencyLevelTaskScheduler.Shutdown()` and always use `CancellationToken` instead? If we do keep the `LimitedConcurrencyLevelTaskScheduler.Shutdown()` method, there are some things to consider: 1. Should we make `LimitedConcurrencyLevelTaskScheduler` public? The BCL doesn't have one and it is provided in the docs for `TaskScheduler`. But there is no `ShutDown()` on that implementation. 2. There are some tests where `Shutdown()` was called in Lucene that would likely perform better if we use it, since those lines were often commented out in Lucene.NET. However, using `CancellationToken` could be an alternative way to handle those cases. - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/core/src/test/org/apache/lucene/search/TestBooleanQuery.java#L177-L178 - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/core/src/test/org/apache/lucene/search/TestSearcherManager.java#L291-L292 - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/suggest/src/test/org/apache/lucene/search/spell/TestSpellChecker.java#L435-L437 - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/test-framework/src/java/org/apache/lucene/index/ThreadedIndexingAndSearchingTestCase.java#L645-L646 - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/core/src/test/org/apache/lucene/search/TestIndexSearcher.java#L119 - https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.1/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java#L1570 The upside of using `Shutdown()` is that it aligns more closely to Lucene. The upside of using `CancellationToken` is that it is more fine-grained and can potentially shut down a process quicker, since it will be able to intervene inside of the running task instead of prior to queuing the task. Allowing a user-defined `TaskScheduler` still seems like a good idea, but since Microsoft didn't allow them to be cancelled, it seems like we should follow the `CancellationToken` approach instead of trying to shoehorn a way to cancel a `TaskScheduler`. Using the same `CancellationToken` to make `LimitedConcurrencyLevelTaskScheduler` stop queuing new work is also something that might be worth considering. -- 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: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
