Fix resource group concurrency for multi coordinator #16956
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swapsmagic
force-pushed
the
fix_resource_group_concurrency
branch
2 times, most recently
from
840cf32
to
9f6a51a
Compare
When a burst of traffic comes to coordinator, it ended up running more than allowed queries. Two reasons for that: 1. In multi coordinator, we were not stamping last running query for non leaf resource groups, which lead to shouldWaitForResourceManagerUpdate to return always true for non leaf resource groups. So if the traffic is coming from lot of different resource groups, coordinator end up running less than allowed in each resource group but at root level it ends up running more. 2. ResourceManagerResourceGroupService cache end up having stale resource group info which also end up allowing coordinators to run more than allowed queries at a cluster level. As part of this diff we are fixing 1 by stamping last running query to all it's parent resource groups. And to address 2, making cache refresh rate and expiration configerable.
swapsmagic
force-pushed
the
fix_resource_group_concurrency
branch
from
9f6a51a
to
7cfc76c
Compare
swapsmagic
requested review from
tdcmeehan,
vaishnavibatni,
abhiseksaikia and
neeradsomanchi
ajaygeorge
reviewed
Nov 9, 2021
| @@ -152,7 +152,7 @@ | |||
| private final CounterStat timeBetweenStartsSec = new CounterStat(); | |||
|
|
|||
| @GuardedBy("root") | |||
| private AtomicLong lastRunningQueryStartTime = new AtomicLong(); | |||
| private AtomicLong lastRunningQueryStartTime = new AtomicLong(currentTimeMillis()); | |||
There was a problem hiding this comment.
can you help me understand why initializing with currentTimeMillis() is important?
There was a problem hiding this comment.
The reason for this is: internal resource groups created as and when needed, and this make sure lastRunningQueryStartTime for the newly created resource group won't be set to 0. This helps make sure we won't end up letting resource group to run more than allowed queries in case RM updates are delayed and we wait for the RM update before running more queries on the resource group.
tdcmeehan
approved these changes
Nov 10, 2021
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a burst of traffic comes to coordinator, it ended up running more than allowed queries.
Two reasons for that:
shouldWaitForResourceManagerUpdate to return always true for non leaf resource groups. So if the traffic is coming from
lot of different resource groups, coordinator end up running less than allowed in each resource group but at root level it
ends up running more.
to run more than allowed queries at a cluster level.
As part of this diff we are fixing 1 by stamping last running query to all it's parent resource groups. And to address 2, making
cache refresh rate and expiration configerable.
Test plan - Unit test + regression test on verifier cluster