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

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

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



##########
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:
       Looks like only StreamingSelectionOnlyCombineOperator use `LinkedBlockingQueue`, all other combine operators use `ArrayBlockingQueue` since the number of blocks can be determined . Since variable `_blockingQueue` is  protected, how about make it as `ArrayBlockingQueue` by default here and in constructor of StreamingSelectionOnlyCombineOperator, override `_blockingQueue` as `LinkedBlockingQueue`?




-- 
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