Do not serialize metrics in each Operator #10473

KKcorps · 2023-03-24T19:31:18Z

There's no need to serialize the metrics unless they are sent or received. We can simply use the OperatorStats objects for rest of the operators.

codecov-commenter · 2023-03-24T20:09:57Z

Codecov Report

Merging #10473 (68c496b) into master (641ec60) will increase coverage by 0.20%.
The diff coverage is 97.77%.

@@             Coverage Diff              @@
##             master   #10473      +/-   ##
============================================
+ Coverage     69.97%   70.17%   +0.20%     
- Complexity     6118     6254     +136     
============================================
  Files          2092     2112      +20     
  Lines        112712   113050     +338     
  Branches      17143    17007     -136     
============================================
+ Hits          78874    79338     +464     
+ Misses        28260    28167      -93     
+ Partials       5578     5545      -33

Flag	Coverage Δ
integration1	`24.31% <0.00%> (-0.22%)`	⬇️
integration2	`24.08% <0.00%> (-0.22%)`	⬇️
unittests1	`67.85% <97.77%> (+0.30%)`	⬆️
unittests2	`13.87% <0.00%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...uery/runtime/executor/OpChainSchedulerService.java	`96.77% <ø> (-0.06%)`	⬇️
...ot/query/runtime/operator/utils/OperatorUtils.java	`83.05% <ø> (-0.56%)`	⬇️
...query/runtime/operator/MailboxReceiveOperator.java	`91.81% <75.00%> (-2.63%)`	⬇️
...va/org/apache/pinot/query/runtime/QueryRunner.java	`80.86% <100.00%> (+0.16%)`	⬆️
...e/operator/LeafStageTransferableBlockOperator.java	`78.09% <100.00%> (+0.21%)`	⬆️
...ot/query/runtime/operator/MailboxSendOperator.java	`77.94% <100.00%> (+2.13%)`	⬆️
...not/query/runtime/operator/MultiStageOperator.java	`78.12% <100.00%> (-1.43%)`	⬇️
...g/apache/pinot/query/runtime/operator/OpChain.java	`100.00% <100.00%> (ø)`
...che/pinot/query/runtime/operator/OpChainStats.java	`91.17% <100.00%> (+4.07%)`	⬆️
...he/pinot/query/runtime/operator/OperatorStats.java	`97.72% <100.00%> (+4.24%)`	⬆️
... and 2 more

... and 210 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

KKcorps · 2023-03-28T13:30:45Z

@walterddr one query: Are you fine if I simply create OpChainStats inside OpChainExecutionContext and then use it everywhere instead of using .attach function introduced here.

The only downside I see is context should be immutable while stats are mutable.

walterddr · 2023-03-29T15:54:08Z

...query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java

+            TransferableBlock eosBlockWithStats = TransferableBlockUtils.getEndOfStreamTransferableBlock(
+                OperatorUtils.getMetadataFromOperatorStats(_opChainStats.getOperatorStatsMap()));
+            _exchange.send(eosBlockWithStats);
+            return eosBlockWithStats;


why do we need to return this instead of just transferableBlock? we don't have any further processing of the stats right?

So that it gets serialized and sent to next OpChain.

_exchange.send(eosBlockWithStats) sends to the next opChain

return eosBlockWithStats is returning to the opChainScheduler.

do we process anything related to stats on opChainScheduler? if not returning the original transferableBlock seems more reasonable

...-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java

walterddr · 2023-03-29T16:04:41Z

...-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java

@@ -39,21 +35,18 @@ public abstract class MultiStageOperator implements Operator<TransferableBlock>,

  // TODO: Move to OperatorContext class.
  protected final OperatorStats _operatorStats;


do we still need this as final reference? can we also put this in OpChainStats and access via OpChainStats?

Yes, we still need it as OperatorStats contains stats only for a single operator.

IIUC, there will be 0 or 1 OperatorStats object corresponding to this operator in the OpChainStats map. if so why do we keep strong reference to both objects in 2 classes?

Cool. So now I have made the change to use directly via the map in OpChainStats rather than creating and storing a strong reference in the base class.

walterddr · 2023-03-29T16:09:50Z

...ry-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java

+              if (_opChainStats != null && !block.getResultMetadata().isEmpty()) {
+                for (Map.Entry<String, OperatorStats> entry : block.getResultMetadata().entrySet()) {
+                  _opChainStats.getOperatorStatsMap().compute(entry.getKey(), (_key, _value) -> entry.getValue());


this is deserializing the metadata from received metadata from the previous stage and putting them into the current ones. if this is true. when there's no trace enabled, skip this step

where is the opChain stats being encoded? or it is also in the map but when trace is enabled there will be more keys in the map?

Tracing thing is being taken care of in the next PR. Need to rebase that PR with the changes in this one.

walterddr

following up my previous comments. also could you fix conflict and CI failures? thank you

walterddr · 2023-03-30T16:04:03Z

...query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java

+            TransferableBlock eosBlockWithStats = TransferableBlockUtils.getEndOfStreamTransferableBlock(
+                OperatorUtils.getMetadataFromOperatorStats(_opChainStats.getOperatorStatsMap()));
+            _exchange.send(eosBlockWithStats);
+            return eosBlockWithStats;


_exchange.send(eosBlockWithStats) sends to the next opChain

return eosBlockWithStats is returning to the opChainScheduler.

do we process anything related to stats on opChainScheduler? if not returning the original transferableBlock seems more reasonable

walterddr · 2023-03-30T16:05:09Z

...-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java

@@ -39,21 +35,18 @@ public abstract class MultiStageOperator implements Operator<TransferableBlock>,

  // TODO: Move to OperatorContext class.
  protected final OperatorStats _operatorStats;


IIUC, there will be 0 or 1 OperatorStats object corresponding to this operator in the OpChainStats map. if so why do we keep strong reference to both objects in 2 classes?

…the send operator

walterddr

lgtm

...query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java

* WIP: Do not serialize metrics * No need to pass stats between operator. Only collected in the end at the send operator * Use opchain stats to record operatorStats * No need to serialie metrics in receive operator * Remove attachStats method and create stats object inside context itself * Make stats thread safe * Add test for opchain stats * Ensure SendOperator stats are populated before serializing stats * Fix variable scope * Use operator stats map directly from opchain stats * unify return statements outside inner for loop in MailboxSendOperator --------- Co-authored-by: Kartik Khare <kharekartik@Kartiks-MacBook-Pro.local>

KKcorps changed the title ~~WIP: Do not serialize metrics~~ Do not serialize metrics Mar 27, 2023

KKcorps requested a review from walterddr March 27, 2023 18:15

KKcorps changed the title ~~Do not serialize metrics~~ Do not serialize metrics in each Operator Mar 28, 2023

KKcorps force-pushed the zero_serialisation_query_metrics branch from 1351cd8 to 3f16d70 Compare March 29, 2023 08:07

walterddr reviewed Mar 29, 2023

View reviewed changes

KKcorps force-pushed the zero_serialisation_query_metrics branch from 8b26fcd to 68a3787 Compare March 30, 2023 09:03

walterddr reviewed Mar 30, 2023

View reviewed changes

Kartik Khare and others added 10 commits March 31, 2023 14:12

WIP: Do not serialize metrics

9430e0f

No need to pass stats between operator. Only collected in the end at …

aab4235

…the send operator

Use opchain stats to record operatorStats

4a85cbf

No need to serialie metrics in receive operator

102c9b1

Remove attachStats method and create stats object inside context itself

be565f5

Make stats thread safe

59c07a5

Add test for opchain stats

bfdde06

Ensure SendOperator stats are populated before serializing stats

27e5a2b

Fix variable scope

f062968

Use operator stats map directly from opchain stats

919eafe

KKcorps force-pushed the zero_serialisation_query_metrics branch from 1838382 to 919eafe Compare March 31, 2023 08:47

walterddr approved these changes Apr 1, 2023

View reviewed changes

...query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java Outdated Show resolved Hide resolved

walterddr mentioned this pull request Apr 1, 2023

[multistage] fix stats/trace #10402

Closed

6 tasks

unify return statements outside inner for loop in MailboxSendOperator

68c496b

KKcorps merged commit 941cbf8 into apache:master Apr 1, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not serialize metrics in each Operator #10473

Do not serialize metrics in each Operator #10473

KKcorps commented Mar 24, 2023 •

edited

codecov-commenter commented Mar 24, 2023 •

edited

KKcorps commented Mar 28, 2023

walterddr Mar 29, 2023

KKcorps Mar 29, 2023

walterddr Mar 30, 2023

walterddr Mar 29, 2023

KKcorps Mar 30, 2023

walterddr Mar 30, 2023

KKcorps Mar 31, 2023

walterddr Mar 29, 2023

KKcorps Mar 29, 2023

walterddr left a comment

walterddr Mar 30, 2023

walterddr Mar 30, 2023

walterddr left a comment

		@@ -39,21 +35,18 @@ public abstract class MultiStageOperator implements Operator<TransferableBlock>,

		// TODO: Move to OperatorContext class.
		protected final OperatorStats _operatorStats;

Do not serialize metrics in each Operator #10473

Do not serialize metrics in each Operator #10473

Conversation

KKcorps commented Mar 24, 2023 • edited

codecov-commenter commented Mar 24, 2023 • edited

Codecov Report

KKcorps commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

KKcorps commented Mar 24, 2023 •

edited

codecov-commenter commented Mar 24, 2023 •

edited