Add consuming metadata to broker response #9092

ravishankar15 · 2022-07-22T13:39:04Z

Added numConsumingSegmentsProcessed, numConsumingSegmentsMatched to broker response metaData

I am assuming,
numConsumingSegmentsProcessed to be a sub set of numSegmentsProcessed
and numConsumingSegmentsMatched to be a sub set of numSegmentsMatched

codecov-commenter · 2022-07-22T14:22:33Z

Codecov Report

Merging #9092 (fda1240) into master (dc2deb8) will increase coverage by 8.65%.
The diff coverage is 95.74%.

@@             Coverage Diff              @@
##             master    #9092      +/-   ##
============================================
+ Coverage     28.52%   37.18%   +8.65%     
- Complexity       51      193     +142     
============================================
  Files          1826     1838      +12     
  Lines         96961    97358     +397     
  Branches      14627    14669      +42     
============================================
+ Hits          27662    36201    +8539     
+ Misses        66672    58224    -8448     
- Partials       2627     2933     +306

Flag	Coverage Δ
integration1	`26.33% <95.74%> (-0.11%)`	⬇️
integration2	`24.59% <95.74%> (-0.12%)`	⬇️
unittests2	`15.29% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...core/operator/blocks/IntermediateResultsBlock.java	`66.51% <80.00%> (+0.65%)`	⬆️
...roker/requesthandler/BaseBrokerRequestHandler.java	`73.31% <100.00%> (+9.39%)`	⬆️
...t/common/response/broker/BrokerResponseNative.java	`91.12% <100.00%> (+0.44%)`	⬆️
.../java/org/apache/pinot/common/utils/DataTable.java	`95.65% <100.00%> (+0.19%)`	⬆️
...ot/core/operator/combine/CombineOperatorUtils.java	`100.00% <100.00%> (ø)`
...core/query/executor/ServerQueryExecutorV1Impl.java	`77.63% <100.00%> (-2.84%)`	⬇️
...che/pinot/core/query/reduce/BaseReduceService.java	`92.14% <100.00%> (+0.43%)`	⬆️
...che/pinot/core/query/scheduler/QueryScheduler.java	`82.55% <100.00%> (+0.60%)`	⬆️
...ntroller/helix/core/minion/TaskMetricsEmitter.java	`34.88% <0.00%> (-51.17%)`	⬇️
...ache/pinot/core/operator/docidsets/OrDocIdSet.java	`86.36% <0.00%> (-13.64%)`	⬇️
... and 320 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

mcvsubbu · 2022-07-22T18:30:27Z

cc: @snleee

PrachiKhobragade

Looks good

snleee

Somehow the regression test failed. Let's fix the issue.

snleee · 2022-07-23T16:46:15Z

pinot-core/src/main/java/org/apache/pinot/core/operator/combine/CombineOperatorUtils.java

Can you remove the try-catch block?

@snleee I am not sure if we can remove that for queries with limit 0 like SELECT * FROM testTable LIMIT 0 we are passing EmptySelectionOperator Instance in there which does not implement the getIndexSegment() hence there will be an exception we have a explicit test case to prevent that SelectionCombineOperatorTest.testSelectionLimit0 Since we are using the interface (Operator) to reference in the for loop and interface has explicit check to throw the exception I think it is better to enclose it within the try catch block.

Also is the regression failing because of this try catch ? I could not see any logs associated with the same ? Can you please help me understand the failure so that I can fix the issue. Thanks

@ravishankar15 I thought getIndexSegment() would return null but the interface actually throw UnsupportedOperationException and you're handling it correctly.

Let me take a look on the regression test and try to check why it fails.

snleee · 2022-07-25T18:08:01Z

@ravishankar15 I don't see that your code change is related to the failure in the compatibility test (it fails on segment upload). Can you try to rebase the code with the upstream master? Let's see if it fixes the issue.

jackjlli · 2022-07-26T06:04:02Z

...-core/src/test/java/org/apache/pinot/core/operator/combine/SelectionCombineOperatorTest.java

Can we rename the below createSegment() method to sth like createOfflineSegment() since you've added a new createRealtimeSegment() method here?

Rename the createSegment method name in test

snleee

It looks that the compatibility test will start to pass after we forcefully merge this PR because the existing job would use the old test set.

@ravishankar15 Thanks you for the contribution!

ravishankar15 · 2022-07-27T12:27:17Z

It looks that the compatibility test will start to pass after we forcefully merge this PR because the existing job would use the old test set.

@ravishankar15 Thanks you for the contribution!

@snleee The error says something like this 2022/07/27 04:02:38.479 ERROR [SegmentOp] [main] Upload segment verification failed, routing table has not been updated after max wait time 60000 ms. It is not related to mismatch I hope can you shed some light here for my understanding. Thanks

snleee · 2022-07-27T18:25:05Z

@ravishankar15 Yeah I also saw that the compatibility test is failing on the routing table update. I will take a look on this and see if we can resolve it without rolling back this PR.

Jackie-Jiang · 2022-07-28T00:10:21Z

pinot-common/src/main/java/org/apache/pinot/common/utils/DataTable.java

    NUM_SEGMENTS_PROCESSED("numSegmentsProcessed", MetadataValueType.INT),
    NUM_SEGMENTS_MATCHED("numSegmentsMatched", MetadataValueType.INT),
    NUM_CONSUMING_SEGMENTS_QUERIED("numConsumingSegmentsQueried", MetadataValueType.INT),
+    NUM_CONSUMING_SEGMENTS_PROCESSED("numConsumingSegmentsProcessed", MetadataValueType.INT),


(MAJOR) Please revert this PR, and re-check it in after it passes the compatibility test. We cannot add MetadataKey in the middle because we use ordinal to encode the key. See the comment of this enum

+1. This is a breaking change

This reverts commit 9cf7d81.

Jackie-Jiang · 2022-07-28T00:17:21Z

Created #9119 to revert this commit for now. We can open a new PR to check it in after fixing the compatible issue. IIRC, the compatibility test won't compare the metadata

This reverts commit 9cf7d81.

snleee · 2022-07-28T00:57:26Z

   * ATTENTION:
   *  - Don't change existing keys.
   *  - Don't remove existing keys.
   *  - Always add new keys to the end.
   *  Otherwise, backward compatibility will be broken.

@ravishankar15 Can you re-file the PR by adding the metadata key to the end?

ravishankar15 · 2022-07-28T04:10:39Z

   * ATTENTION:
   *  - Don't change existing keys.
   *  - Don't remove existing keys.
   *  - Always add new keys to the end.
   *  Otherwise, backward compatibility will be broken.

@ravishankar15 Can you re-file the PR by adding the metadata key to the end?

Yeah sure @snleee I will raise a separate PR with all the changes

ravishankar15 force-pushed the add-metadata-broker-response branch from 37579da to d0c75e4 Compare July 22, 2022 17:26

PrachiKhobragade approved these changes Jul 22, 2022

View reviewed changes

snleee reviewed Jul 23, 2022

View reviewed changes

ravishankar15 force-pushed the add-metadata-broker-response branch from d0c75e4 to df9dd23 Compare July 25, 2022 18:13

jackjlli reviewed Jul 26, 2022

View reviewed changes

ravishankar15 added 2 commits July 26, 2022 18:05

Add consuming metadata to broker response

0fa893c

Rename the createSegment method name in test

Fix compatibility failures

fda1240

ravishankar15 force-pushed the add-metadata-broker-response branch from 8deda42 to fda1240 Compare July 26, 2022 12:36

snleee approved these changes Jul 27, 2022

View reviewed changes

snleee merged commit 9cf7d81 into apache:master Jul 27, 2022

Jackie-Jiang reviewed Jul 28, 2022

View reviewed changes

Jackie-Jiang added a commit to Jackie-Jiang/pinot that referenced this pull request Jul 28, 2022

Revert "Add consuming metadata to broker response (apache#9092)"

e22afa8

This reverts commit 9cf7d81.

Jackie-Jiang mentioned this pull request Jul 28, 2022

Revert "Add consuming metadata to broker response (#9092)" #9119

Merged

Jackie-Jiang added a commit that referenced this pull request Jul 28, 2022

Revert "Add consuming metadata to broker response (#9092)" (#9119)

fc696ce

This reverts commit 9cf7d81.

ravishankar15 mentioned this pull request Jul 28, 2022

Add additional metadata to the response #9122

Merged

Jackie-Jiang mentioned this pull request Aug 4, 2022

Add id to MetadataKey so that we can change the key order #9159

Merged

vvivekiyer mentioned this pull request Sep 29, 2023

Reduce cpu usage by avoiding throwing an exception during query execution #11715

Merged

Add consuming metadata to broker response #9092

Add consuming metadata to broker response #9092

Uh oh!

Conversation

ravishankar15 commented Jul 22, 2022

Uh oh!

codecov-commenter commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcvsubbu commented Jul 22, 2022

Uh oh!

PrachiKhobragade left a comment

Choose a reason for hiding this comment

Uh oh!

snleee left a comment

Choose a reason for hiding this comment

Uh oh!

snleee Jul 23, 2022

Choose a reason for hiding this comment

Uh oh!

ravishankar15 Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

snleee Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

snleee commented Jul 25, 2022

Uh oh!

jackjlli Jul 26, 2022

Choose a reason for hiding this comment

Uh oh!

snleee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravishankar15 commented Jul 27, 2022

Uh oh!

snleee commented Jul 27, 2022

Uh oh!

Jackie-Jiang Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang commented Jul 28, 2022

Uh oh!

snleee commented Jul 28, 2022

Uh oh!

ravishankar15 commented Jul 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-commenter commented Jul 22, 2022 •

edited

Loading

snleee left a comment •

edited

Loading