[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6696: Extract more common logic in combine operators and extend StreamingSelectionOnlyCombineOperator from BaseCombineOperator

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #6696:
URL: https://github.com/apache/incubator-pinot/pull/6696#discussion_r597350972



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java
##########
@@ -50,42 +50,41 @@
   private static final Logger LOGGER = LoggerFactory.getLogger(BaseCombineOperator.class);
 
   protected final List<Operator> _operators;
+  protected final int _numOperators;
   protected final QueryContext _queryContext;
   protected final ExecutorService _executorService;
   protected final long _endTimeMs;
-  protected final int _numOperators;
-  // Use a Phaser to ensure all the Futures are done (not scheduled, finished or interrupted) before the main thread
-  // returns. We need to ensure this because the main thread holds the reference to the segments. If a segment is
-  // deleted/refreshed, the segment will be released after the main thread returns, which would lead to undefined
-  // behavior (even JVM crash) when processing queries against it.
-  protected final Phaser _phaser = new Phaser(1);
-  // Use a _blockingQueue to store the per-segment result
-  protected final BlockingQueue<IntermediateResultsBlock> _blockingQueue;
-  private final AtomicLong totalWorkerThreadCpuTimeNs = new AtomicLong(0);
-  protected int _numThreads;
-  protected Future[] _futures;
-
-  public BaseCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
-      long endTimeMs) {
+  protected final int _numThreads;
+  protected final Future[] _futures;
+  // Use a _blockingQueue to store the intermediate results blocks
+  protected final BlockingQueue<IntermediateResultsBlock> _blockingQueue = new LinkedBlockingQueue<>();

Review comment:
       I considered that, but decide to use `LinkedBlockingQueue` as the default for the following reasons:
   - `ArrayBlockingQueue` pre-allocates all the memory, which might not be needed, and has larger memory footprint
   - `LinkedBlockingQueue` has better throughput because it uses separate locks for the head and the tail
   - We don't need the customized fairness policy provided by the `ArrayBlockingQueue`
   
   Here is the reference link: https://stackoverflow.com/questions/35967792/when-to-prefer-linkedblockingqueue-over-arrayblockingqueue




-- 
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: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org