Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow getting metrics after the operator is closed #9615

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Allow getting metrics after the operator is closed #9615

merged 1 commit into from
Nov 8, 2021

Conversation

rzeyde-varada
Copy link
Contributor

In case the metrics are updated when close() is called, the metrics' handling code would be simpler.

Following #6232 (comment).

@cla-bot cla-bot bot added the cla-signed label Oct 13, 2021
@findepi
Copy link
Member

findepi commented Oct 14, 2021

Does this relate to #8691 ?

@rzeyde-varada
Copy link
Contributor Author

rzeyde-varada commented Oct 17, 2021 •
edited
Loading

No, it's not related - we suggest this change to simplify the implementation when the metrics should be updated on ConnectorPageSource#close.

@rzeyde-varada
Copy link
Contributor Author

Ping :)

@rzeyde-varada rzeyde-varada requested a review from sopel39 November 5, 2021 09:21
sopel39
sopel39 reviewed Nov 5, 2021
View reviewed changes
core/trino-main/src/main/java/io/trino/operator/WorkProcessorPipelineSourceOperator.java
@@ -538,6 +538,7 @@ private void closeOperators(int lastOperatorIndex)
operatorContext.getDriverContext().getTaskId());
}
finally {
workProcessorOperatorContext.metrics.set(operator.getMetrics());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also set connectorMetrics (if its first operator in pipeline) (+ test)

BTW: I think you can just modify io.trino.operator.WorkProcessorPipelineSourceOperator#workProcessorOperatorStateMonitor method to report metrics after:

if (state.getType() == FINISHED) {
..
}

if

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also set connectorMetrics (if its first operator in pipeline) (+ test)

Thanks, added in 2c2aca64e9.

I think you can just modify io.trino.operator.WorkProcessorPipelineSourceOperator#workProcessorOperatorStateMonitor method

I am not sure, since closeOperators is dropping the closed operators in the finally clause:

trino/core/trino-main/src/main/java/io/trino/operator/WorkProcessorPipelineSourceOperator.java

Line 543 in 43e5836

workProcessorOperatorContext.operator = null;

Please let me know if you meant to refactor closeOperators method as well.

@rzeyde-varada
Allow getting metrics after the operator is closed
d49ed1c 
In case the metrics are updated when close() is called, the metrics' handling code would be simpler.
@rzeyde-varada
Copy link
Contributor Author

Rebased over latest master.

@sopel39 sopel39 merged commit 320f82e into trinodb:master Nov 8, 2021
@sopel39 sopel39 mentioned this pull request Nov 8, 2021
Closed
12 tasks
@github-actions github-actions bot added this to the 365 milestone Nov 8, 2021
@rzeyde-varada rzeyde-varada deleted the metrics-after-close branch November 9, 2021 06:35
@rzeyde-varada
Copy link
Contributor Author

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sopel39 sopel39 sopel39 approved these changes

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

Assignees
No one assigned
Labels
cla-signed
Milestone
365
Development

Successfully merging this pull request may close these issues.
3 participants
@rzeyde-varada @findepi @sopel39