-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Add toggle for specifying dynamic filtering refresh interval #4388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what is the config parameters deprecation policy. E.g If plan to remove it soon anyway should we maybe already mark it as @deprecated?
Also I was not able to understand motivation for the change based on commit message :)
a1a2fe2
to
4fcca0c
Compare
I think
I've improved it slightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise it looks good. I would prefer extra pair of eyes on property name. @electrum ?
@@ -786,6 +790,21 @@ public FeaturesConfig setDynamicFilteringMaxPerDriverSize(DataSize dynamicFilter | |||
return this; | |||
} | |||
|
|||
@MinDuration("1ms") | |||
@MaxDuration("10s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: testcase for validation?
@@ -131,6 +134,7 @@ | |||
private boolean enableDynamicFiltering = true; | |||
private int dynamicFilteringMaxPerDriverRowCount = 100; | |||
private DataSize dynamicFilteringMaxPerDriverSize = DataSize.of(10, KILOBYTE); | |||
private Duration dynamicFilteringRefreshInterval = new Duration(100, MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the default previously? Arent we changing rate too aggressively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The earlier default was 1 sec. It might be worth checking the time taken by collectDynamicFilters when there are large no. of queries active with multiple stages containing dynamic filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to 200ms. I don't think it's too aggressive. Anyway, it's limited to single thread.
4fcca0c
to
002f1a5
Compare
Previously, dynamic filtering refresh interval was set to "task.status-refresh-max-wait". However, task status will be refreshed more frequently since "task.status-refresh-max-wait" specifies maximum waiting time. Therefore a dedicated refresh toggle is added that makes dynamic filtering collection more frequent. This toggle is marked as experimental as it will eventually be replaced by delta-based task status update mechanism.
002f1a5
to
44d2fef
Compare
Previously, dynamic filtering refresh interval was set to
"task.status-refresh-max-wait". However, task status will be
refreshed more frequently since "task.status-refresh-max-wait"
specifies max wait. Therefore a dedicated refresh toggle is added.
This toggle is marked as experimental as it will eventually be replaced by
delta-based task status update mechanism.