You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/09/18 01:47:09 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request #7450: Unify CombineOperator multi-threading logic
Jackie-Jiang opened a new pull request #7450:
URL: https://github.com/apache/pinot/pull/7450
Unify the logic of using multiple threads to process segments within the combine operators
- Make group-by combine operators follow the `numThreads` limit so that we can control it's thread usage
- Extract common logic in combine operators and simplify the actual implementation
NOTE: This PR doesn't limit the thread usage for group-by combine operator to keep the existing behavior, but only make it possible
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-922169843
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7450](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb046af) into [master](https://codecov.io/gh/apache/pinot/commit/0069fc3545b558cf7e49bccac4bf37a614433e48?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0069fc3) will **increase** coverage by `8.54%`.
> The diff coverage is `77.33%`.
[](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7450 +/- ##
============================================
+ Coverage 62.43% 70.97% +8.54%
- Complexity 3371 3377 +6
============================================
Files 1508 1517 +9
Lines 74883 75212 +329
Branches 10925 10964 +39
============================================
+ Hits 46751 53385 +6634
+ Misses 24746 18192 -6554
- Partials 3386 3635 +249
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.41% <74.66%> (?)` | |
| integration2 | `?` | |
| unittests1 | `69.83% <65.33%> (+0.02%)` | :arrow_up: |
| unittests2 | `14.54% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-62.75%)` | :arrow_down: |
| [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `79.66% <77.77%> (+2.61%)` | :arrow_up: |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `72.80% <78.94%> (+2.65%)` | :arrow_up: |
| [...perator/combine/GroupByOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlPcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `84.81% <88.88%> (+2.09%)` | :arrow_up: |
| [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `93.15% <100.00%> (+8.42%)` | :arrow_up: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...ator/streaming/StreamingSelectionOnlyOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-92.31%)` | :arrow_down: |
| [...he/pinot/core/plan/StreamingSelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| ... and [313 more](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0069fc3...eb046af](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r713481255
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -125,43 +119,41 @@ 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) {
Review comment:
The existing code in the group-by combine operator has the assumption that one thread only processes one segment. This PR removes that assumption, and each thread can process multiple segments. Currently we still use `_numOperators` threads to process the query, but if we give less threads, it still works
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r712465217
##########
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:
Having said that, `IndexedTable` is _mostly_ immutable so it might be ok in practice https://github.com/apache/pinot/blob/f3068bc93165e359cc3d846c40f62312ecc98af3/pinot-core/src/main/java/org/apache/pinot/core/data/table/IndexedTable.java#L39L46
(I think this can be resolved)
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r711524176
##########
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:
Because `_indexedTable` isn't volatile, double checked locking doesn't work. The JMM allows reordering of the checks and lock.
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r711524613
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/streaming/StreamingSelectionOnlyCombineOperator.java
##########
@@ -71,30 +70,19 @@ public String getOperatorName() {
@Override
protected void processSegments(int threadIndex) {
- for (int operatorIndex = threadIndex; operatorIndex < _numOperators; operatorIndex += _numTasks) {
+ for (int operatorIndex = threadIndex; operatorIndex < _numOperators; operatorIndex += _numThreads) {
Operator<IntermediateResultsBlock> operator = _operators.get(operatorIndex);
- try {
- IntermediateResultsBlock resultsBlock;
- while ((resultsBlock = operator.nextBlock()) != null) {
- Collection<Object[]> rows = resultsBlock.getSelectionResult();
- assert rows != null;
- long numRowsCollected = _numRowsCollected.addAndGet(rows.size());
- _blockingQueue.offer(resultsBlock);
- if (numRowsCollected >= _limit) {
- return;
- }
+ IntermediateResultsBlock resultsBlock;
+ while ((resultsBlock = operator.nextBlock()) != null) {
+ Collection<Object[]> rows = resultsBlock.getSelectionResult();
+ assert rows != null;
+ long numRowsCollected = _numRowsCollected.addAndGet(rows.size());
+ _blockingQueue.offer(resultsBlock);
Review comment:
This logic is wrong - `LinkedBlockingQueue.offer` can return false, but is used as if the offer must succeed in several places, here, the number of rows collected should only be increased when the offer succeeds (though I suspect all of this code is written under the conception that the offers can't fail).
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
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-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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-922169843
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7450](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb046af) into [master](https://codecov.io/gh/apache/pinot/commit/0069fc3545b558cf7e49bccac4bf37a614433e48?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0069fc3) will **increase** coverage by `9.54%`.
> The diff coverage is `85.33%`.
[](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7450 +/- ##
============================================
+ Coverage 62.43% 71.97% +9.54%
- Complexity 3371 3377 +6
============================================
Files 1508 1517 +9
Lines 74883 75212 +329
Branches 10925 10964 +39
============================================
+ Hits 46751 54135 +7384
+ Misses 24746 17437 -7309
- Partials 3386 3640 +254
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.41% <74.66%> (?)` | |
| integration2 | `28.94% <76.00%> (-0.16%)` | :arrow_down: |
| unittests1 | `69.83% <65.33%> (+0.02%)` | :arrow_up: |
| unittests2 | `14.54% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `70.45% <66.66%> (+7.70%)` | :arrow_up: |
| [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `79.66% <77.77%> (+2.61%)` | :arrow_up: |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `75.20% <78.94%> (+5.05%)` | :arrow_up: |
| [...perator/combine/GroupByOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlPcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `84.81% <88.88%> (+2.09%)` | :arrow_up: |
| [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `93.15% <100.00%> (+8.42%)` | :arrow_up: |
| [...l/segment/index/readers/OnHeapFloatDictionary.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvT25IZWFwRmxvYXREaWN0aW9uYXJ5LmphdmE=) | `90.00% <0.00%> (-5.00%)` | :arrow_down: |
| [...rg/apache/pinot/ingestion/jobs/BaseSegmentJob.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vam9icy9CYXNlU2VnbWVudEpvYi5qYXZh) | `31.57% <0.00%> (ø)` | |
| [...pache/pinot/ingestion/jobs/SegmentCreationJob.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vam9icy9TZWdtZW50Q3JlYXRpb25Kb2IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...not/ingestion/common/DefaultControllerRestApi.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vY29tbW9uL0RlZmF1bHRDb250cm9sbGVyUmVzdEFwaS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...org/apache/pinot/ingestion/utils/PushLocation.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vdjBfZGVwcmVjYXRlZC9waW5vdC1pbmdlc3Rpb24tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9pbmdlc3Rpb24vdXRpbHMvUHVzaExvY2F0aW9uLmphdmE=) | `72.72% <0.00%> (ø)` | |
| ... and [265 more](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0069fc3...eb046af](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r712465217
##########
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:
Having said that, `IndexedTable` is _mostly_ immutable so it might be ok in practice https://github.com/apache/pinot/blob/f3068bc93165e359cc3d846c40f62312ecc98af3/pinot-core/src/main/java/org/apache/pinot/core/data/table/IndexedTable.java#L39L46
--
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
[GitHub] [pinot] npawar commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r713458027
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -125,43 +119,41 @@ 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) {
Review comment:
So when you say `This PR doesn't limit the thread usage for group-by combine operator to keep the existing behavior, but only make it possible` you mean that we now have option to pass a different numThreads to this method? And that was completely impossible previously?
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-924256834
> BTW how did the coverage improve so much? I haven't fully understood the Codecov reports yet.
I think it is caused by a recent master build test failure/cancellation. Currently we split the tests into 4 parts, and each of them upload the code coverage report independently. When only partial of them succeeded, we will get inaccurate codecov. Ideally we should only generate percentage when all 4 reports are received.
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-922169843
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7450](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb046af) into [master](https://codecov.io/gh/apache/pinot/commit/0069fc3545b558cf7e49bccac4bf37a614433e48?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0069fc3) will **decrease** coverage by `4.67%`.
> The diff coverage is `65.33%`.
[](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7450 +/- ##
============================================
- Coverage 69.81% 65.13% -4.68%
- Complexity 3286 3377 +91
============================================
Files 1124 1472 +348
Lines 53194 73407 +20213
Branches 8013 10772 +2759
============================================
+ Hits 37136 47812 +10676
- Misses 13425 22197 +8772
- Partials 2633 3398 +765
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `69.83% <65.33%> (+0.02%)` | :arrow_up: |
| unittests2 | `14.54% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `62.40% <57.89%> (+3.44%)` | :arrow_up: |
| [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `83.56% <75.00%> (-1.17%)` | :arrow_down: |
| [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `79.66% <77.77%> (+2.61%)` | :arrow_up: |
| [...perator/combine/GroupByOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlPcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `84.81% <88.88%> (+2.09%)` | :arrow_up: |
| [...l/segment/index/readers/OnHeapFloatDictionary.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvT25IZWFwRmxvYXREaWN0aW9uYXJ5LmphdmE=) | `90.00% <0.00%> (-5.00%)` | :arrow_down: |
| [...roller/recommender/rules/impl/BloomFilterRule.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0Jsb29tRmlsdGVyUnVsZS5qYXZh) | `100.00% <0.00%> (ø)` | |
| [...ot/controller/helix/core/util/ZKMetadataUtils.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3V0aWwvWktNZXRhZGF0YVV0aWxzLmphdmE=) | `90.00% <0.00%> (ø)` | |
| [...che/pinot/controller/api/access/AccessControl.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvYWNjZXNzL0FjY2Vzc0NvbnRyb2wuamF2YQ==) | `30.00% <0.00%> (ø)` | |
| [...rokerResourceOnlineOfflineStateModelGenerator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhCcm9rZXJSZXNvdXJjZU9ubGluZU9mZmxpbmVTdGF0ZU1vZGVsR2VuZXJhdG9yLmphdmE=) | `100.00% <0.00%> (ø)` | |
| ... and [344 more](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0069fc3...eb046af](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r712606625
##########
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:
Thanks for sharing this article. Learned a lot from it! Changed it to `volatile`
--
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
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-922169843
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7450](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c932497) into [master](https://codecov.io/gh/apache/pinot/commit/2c74f203c19d88ea2ed536365d9ce42b73565682?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c74f20) will **increase** coverage by `1.21%`.
> The diff coverage is `86.07%`.
[](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7450 +/- ##
============================================
+ Coverage 70.77% 71.99% +1.21%
- Complexity 3365 3367 +2
============================================
Files 1519 1519
Lines 75335 75327 -8
Branches 10980 10980
============================================
+ Hits 53321 54230 +909
+ Misses 18371 17440 -931
- Partials 3643 3657 +14
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.62% <75.94%> (?)` | |
| integration2 | `29.12% <77.21%> (-0.01%)` | :arrow_down: |
| unittests1 | `69.83% <67.08%> (-0.01%)` | :arrow_down: |
| unittests2 | `14.52% <0.00%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `70.45% <66.66%> (+7.70%)` | :arrow_up: |
| [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `79.66% <77.77%> (+2.61%)` | :arrow_up: |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `74.40% <78.94%> (+3.50%)` | :arrow_up: |
| [...perator/combine/GroupByOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlPcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `84.81% <90.90%> (+2.09%)` | :arrow_up: |
| [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `93.15% <100.00%> (+8.42%)` | :arrow_up: |
| [...nt/local/startree/v2/store/StarTreeDataSource.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZURhdGFTb3VyY2UuamF2YQ==) | `40.00% <0.00%> (-13.34%)` | :arrow_down: |
| [...ot/segment/local/startree/OffHeapStarTreeNode.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9PZmZIZWFwU3RhclRyZWVOb2RlLmphdmE=) | `72.22% <0.00%> (-5.56%)` | :arrow_down: |
| [...org/apache/pinot/spi/plugin/PluginClassLoader.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcGx1Z2luL1BsdWdpbkNsYXNzTG9hZGVyLmphdmE=) | `16.39% <0.00%> (-2.76%)` | :arrow_down: |
| [.../pinot/core/query/scheduler/PriorityScheduler.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUHJpb3JpdHlTY2hlZHVsZXIuamF2YQ==) | `80.82% <0.00%> (-2.74%)` | :arrow_down: |
| [...cal/startree/v2/builder/BaseSingleTreeBuilder.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL0Jhc2VTaW5nbGVUcmVlQnVpbGRlci5qYXZh) | `88.28% <0.00%> (-0.91%)` | :arrow_down: |
| ... and [102 more](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2c74f20...c932497](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7450:
URL: https://github.com/apache/pinot/pull/7450
--
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
[GitHub] [pinot] richardstartin commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r711524613
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/streaming/StreamingSelectionOnlyCombineOperator.java
##########
@@ -71,30 +70,19 @@ public String getOperatorName() {
@Override
protected void processSegments(int threadIndex) {
- for (int operatorIndex = threadIndex; operatorIndex < _numOperators; operatorIndex += _numTasks) {
+ for (int operatorIndex = threadIndex; operatorIndex < _numOperators; operatorIndex += _numThreads) {
Operator<IntermediateResultsBlock> operator = _operators.get(operatorIndex);
- try {
- IntermediateResultsBlock resultsBlock;
- while ((resultsBlock = operator.nextBlock()) != null) {
- Collection<Object[]> rows = resultsBlock.getSelectionResult();
- assert rows != null;
- long numRowsCollected = _numRowsCollected.addAndGet(rows.size());
- _blockingQueue.offer(resultsBlock);
- if (numRowsCollected >= _limit) {
- return;
- }
+ IntermediateResultsBlock resultsBlock;
+ while ((resultsBlock = operator.nextBlock()) != null) {
+ Collection<Object[]> rows = resultsBlock.getSelectionResult();
+ assert rows != null;
+ long numRowsCollected = _numRowsCollected.addAndGet(rows.size());
+ _blockingQueue.offer(resultsBlock);
Review comment:
This logic is wrong - `LinkedBlockingQueue.offer` can return false, but is used as if the offer must succeed in several places, here, the number of rows collected should only be increased when the offer succeeds (though I suspect all of this code is written under the conception that the offers can't fail).
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-922169843
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7450](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (eb046af) into [master](https://codecov.io/gh/apache/pinot/commit/0069fc3545b558cf7e49bccac4bf37a614433e48?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0069fc3) will **increase** coverage by `0.02%`.
> The diff coverage is `65.33%`.
[](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7450 +/- ##
============================================
+ Coverage 69.81% 69.83% +0.02%
+ Complexity 3286 3285 -1
============================================
Files 1124 1124
Lines 53194 53175 -19
Branches 8013 8014 +1
============================================
- Hits 37136 37134 -2
+ Misses 13425 13407 -18
- Partials 2633 2634 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `69.83% <65.33%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...nMaxValueBasedSelectionOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL01pbk1heFZhbHVlQmFzZWRTZWxlY3Rpb25PcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `62.40% <57.89%> (+3.44%)` | :arrow_up: |
| [...not/core/operator/combine/BaseCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Jhc2VDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `83.56% <75.00%> (-1.17%)` | :arrow_down: |
| [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `79.66% <77.77%> (+2.61%)` | :arrow_up: |
| [...perator/combine/GroupByOrderByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlPcmRlckJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `84.81% <88.88%> (+2.09%)` | :arrow_up: |
| [...l/segment/index/readers/OnHeapFloatDictionary.java](https://codecov.io/gh/apache/pinot/pull/7450/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvT25IZWFwRmxvYXREaWN0aW9uYXJ5LmphdmE=) | `90.00% <0.00%> (-5.00%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0069fc3...eb046af](https://codecov.io/gh/apache/pinot/pull/7450?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r712426897
##########
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 will enforce the memory synchronization, thus guarantee that the inner check getting the latest value. I don't think JVM can reorder the check and lock because of the memory barrier.
More can be read here under the `Memory Synchronization` section: https://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/Lock.html
--
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
[GitHub] [pinot] richardstartin commented on pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#issuecomment-923858764
BTW how did the coverage improve so much? I haven't fully understood the Codecov reports yet.
--
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
[GitHub] [pinot] npawar commented on a change in pull request #7450: Unify CombineOperator multi-threading logic
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #7450:
URL: https://github.com/apache/pinot/pull/7450#discussion_r713457443
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -125,43 +119,41 @@ 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) {
Review comment:
in group by case, _numOperators will continue to be equal to _numThreads?
--
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