richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r712463571
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -125,43 +123,44 @@ public String getOperatorName() {
*/
@Override
protected void processSegments(int threadIndex) {
- try {
- IntermediateResultsBlock intermediateResultsBlock =
- (IntermediateResultsBlock) _operators.get(threadIndex).nextBlock();
-
- _initLock.lock();
- try {
- if (_dataSchema == null) {
- _dataSchema = intermediateResultsBlock.getDataSchema();
- // NOTE: Use trimSize as resultSize on server size.
- if (_trimThreshold >= MAX_TRIM_THRESHOLD) {
- // special case of trim threshold where it is set to max value.
- // there won't be any trimming during upsert in this case.
- // thus we can avoid the overhead of read-lock and write-lock
- // in the upsert method.
- _indexedTable = new UnboundedConcurrentIndexedTable(_dataSchema,
_queryContext, _trimSize);
- } else {
- _indexedTable =
- new ConcurrentIndexedTable(_dataSchema, _queryContext,
_trimSize, _trimSize, _trimThreshold);
+ for (int operatorIndex = threadIndex; operatorIndex < _numOperators;
operatorIndex += _numThreads) {
+ IntermediateResultsBlock resultsBlock = (IntermediateResultsBlock)
_operators.get(operatorIndex).nextBlock();
+
+ if (_indexedTable == null) {
+ _initLock.lock();
+ try {
+ if (_indexedTable == null) {
Review comment:
Lock acquisition is guaranteed to happen before the second check, but
the first check can be reordered with the lock acquisition, which nullifies the
benefits of the idiom.
Since `_indexedTable` isn't immutable, there is also the issue of unsafe
publication. It's worth taking a look here, where the example uses
`synchronized` but applies equally to `ReentrantLock`:
https://shipilev.net/blog/2014/safe-public-construction/
Making `_indexedTable` makes the problem go away though.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -125,43 +123,44 @@ public String getOperatorName() {
*/
@Override
protected void processSegments(int threadIndex) {
- try {
- IntermediateResultsBlock intermediateResultsBlock =
- (IntermediateResultsBlock) _operators.get(threadIndex).nextBlock();
-
- _initLock.lock();
- try {
- if (_dataSchema == null) {
- _dataSchema = intermediateResultsBlock.getDataSchema();
- // NOTE: Use trimSize as resultSize on server size.
- if (_trimThreshold >= MAX_TRIM_THRESHOLD) {
- // special case of trim threshold where it is set to max value.
- // there won't be any trimming during upsert in this case.
- // thus we can avoid the overhead of read-lock and write-lock
- // in the upsert method.
- _indexedTable = new UnboundedConcurrentIndexedTable(_dataSchema,
_queryContext, _trimSize);
- } else {
- _indexedTable =
- new ConcurrentIndexedTable(_dataSchema, _queryContext,
_trimSize, _trimSize, _trimThreshold);
+ for (int operatorIndex = threadIndex; operatorIndex < _numOperators;
operatorIndex += _numThreads) {
+ IntermediateResultsBlock resultsBlock = (IntermediateResultsBlock)
_operators.get(operatorIndex).nextBlock();
+
+ if (_indexedTable == null) {
+ _initLock.lock();
+ try {
+ if (_indexedTable == null) {
Review comment:
Lock acquisition is guaranteed to happen before the second check, but
the first check can be reordered with the lock acquisition, which nullifies the
benefits of the idiom.
Since `_indexedTable` isn't immutable, there is also the issue of unsafe
publication. It's worth taking a look here, where the example uses
`synchronized` but applies equally to `ReentrantLock`:
https://shipilev.net/blog/2014/safe-public-construction/
Making `_indexedTable` volatile makes the problem go away though.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
