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

Report dynamic filtering stats in QueryStats #4440

Merged
merged 1 commit into from Aug 20, 2020
Merged

Report dynamic filtering stats in QueryStats #4440

merged 1 commit into from Aug 20, 2020

Conversation

raunaqmorarka
Copy link
Member

@raunaqmorarka raunaqmorarka commented Jul 13, 2020 •
edited by sopel39

Fixes: #3005

@cla-bot cla-bot bot added the cla-signed label Jul 13, 2020
sopel39
sopel39 reviewed Jul 29, 2020
View reviewed changes
presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/server/DynamicFilterService.java Outdated Show resolved Hide resolved
presto-spi/src/main/java/io/prestosql/spi/predicate/SortedRangeSet.java Outdated Show resolved Hide resolved
presto-spi/src/main/java/io/prestosql/spi/predicate/Ranges.java Outdated Show resolved Hide resolved
@sopel39
Copy link
Member

sopel39 commented Jul 31, 2020 •
edited

Could we add an "integration" test case where we run actual query and assert DFs at the end. That would ensure we don't have problem with finalQueryInfo

sopel39
sopel39 approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

small comments

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Outdated
@@ -666,6 +671,17 @@ public void setRoutines(List<RoutineInfo> routines)
this.routines.set(ImmutableList.copyOf(routines));
}

private synchronized DynamicFiltersStats getDynamicFiltersStats()
Copy link
Member

Choose a reason for hiding this comment

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

remote this method and inline dynamicFiltersStatsProvider.get()

Copy link
Member Author

Choose a reason for hiding this comment

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

I avoided AtomicReference<Supplier<DynamicFiltersStats>> because I think there is a potential for race condition with that. Example
In getQueryStats, we get reference to the non-final df stats supplier, but the get on supplier isn't executed yet.
Query finishes, unregisterDynamicFilteringQuery is called, final df stats supplier is set and dynamicFilterService.removeQuery runs.
Now we continue with execution of getQueryStats, we still have reference to old supplier and calling get on it will give empty stats now as the dynamic filter service has cleaned up its maps.

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense, thanks

presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/execution/QueryStateMachine.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/execution/SqlQueryExecution.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/server/TestDynamicFilterService.java Outdated Show resolved Hide resolved
@sopel39 sopel39 merged commit 9d26aea into trinodb:master Aug 20, 2020
@sopel39
Copy link
Member

sopel39 commented Aug 20, 2020

merged thanks!

@sopel39 sopel39 mentioned this pull request Aug 20, 2020
Closed
9 tasks
@raunaqmorarka raunaqmorarka deleted the dfs_stats branch August 20, 2020 11:02
@martint martint added this to the 341 milestone Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martint martint Awaiting requested review from martint

@Lewuathe Lewuathe Awaiting requested review from Lewuathe

@sopel39 sopel39 Awaiting requested review from sopel39

Assignees
No one assigned
Labels
cla-signed
Milestone
341
Development

Successfully merging this pull request may close these issues.

Report dynamic filtering stats for DynamicFilterSourceOperator and scan operator
4 participants
@raunaqmorarka @sopel39 @Lewuathe @martint