-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 support for memoization to resource group state endpoint #22764
Add support for memoization to resource group state endpoint #22764
Conversation
95842e2
to
c0f6d27
Compare
c0f6d27
to
dbecb4e
Compare
c097be5
to
272fd92
Compare
Nit, request add PR number to the release note entry.
|
@swapsmagic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
presto-main/src/main/java/com/facebook/presto/server/ResourceGroupStateInfoResource.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/ServerConfig.java
Outdated
Show resolved
Hide resolved
@@ -76,6 +103,9 @@ public ResourceGroupStateInfoResource( | |||
this.resourceGroupManager = requireNonNull(resourceGroupManager, "resourceGroupManager is null"); | |||
this.internalNodeManager = requireNonNull(internalNodeManager, "internalNodeManager is null"); | |||
this.proxyHelper = requireNonNull(proxyHelper, "proxyHelper is null"); | |||
this.resourceGroupStateInfoKeySupplierMap = new HashMap<>(); | |||
this.expirationDuration = requireNonNull(serverConfig, "serverConfig is null").getResourceGroupStateInfoExpirationDuration(); | |||
this.rootResourceGroupInfoSupplier = expirationDuration.getValue() > 0 ? memoizeWithExpiration(() -> resourceGroupManager.getRootResourceGroups(), expirationDuration.toMillis(), MILLISECONDS) : Suppliers.ofInstance(resourceGroupManager.getRootResourceGroups()); |
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.
Very long line. Lets break it with ? and :
presto-main/src/main/java/com/facebook/presto/server/ResourceGroupStateInfoResource.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/ResourceGroupStateInfoResource.java
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/server/TestResourceGroupStateInfoResource.java
Outdated
Show resolved
Hide resolved
3ca9aab
to
1f0f968
Compare
@swapsmagic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1f0f968
to
dd37434
Compare
@swapsmagic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
presto-tests/src/test/java/com/facebook/presto/server/TestResourceGroupStateInfoResource.java
Show resolved
Hide resolved
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.
Except for the unit test comment. The changes look good to me.
dd37434
to
130a3be
Compare
@swapsmagic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Adding memoization support to /v1/resourceGroupState endpoint. This will help reduce load on the coordinator to calculate the resource group state for concurrent/frequent requests in a short period of time.
130a3be
to
dbf6c37
Compare
@swapsmagic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR #22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR #22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
As part of the memoization introduction, previous PR prestodb#22764 introduced an issue where the endpoint returns the same value all the time when caching is disabled. Fixing it with this change and introducing unit tests to cover both caching enabled and not enabled scenario to verify the result.
Description
Adding memoization support to /v1/resourceGroupState endpoint.
Motivation and Context
This will help reduce load on the coordinator to
calculate the resource group state for concurrent/frequent requests in a short period of time.
Impact
Less contention on coordinator from /v1/resourceGroupState endpoint
Test Plan
unit test
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.