-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Cache Block Location to save memory #16953
Conversation
@maobaolong yup thanks |
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.
LGTM with a minor comment, thanks!
core/common/src/main/java/alluxio/util/proto/BlockLocationUtils.java
Outdated
Show resolved
Hide resolved
48086a9
to
a5b6082
Compare
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.
This PR can help to reduce heap cost extremely, but there are hard code about MediumType and tier alias.
private static final IndexDefinition<BlockLocation, Long> WORKER_ID_INDEX = | ||
IndexDefinition.ofNonUnique(BlockLocation::getWorkerId); | ||
|
||
// TODO(maobaolong): Add a metric to monitor the size of mLocationCacheMap |
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.
As mLocationCacheMap
renamed to BLOCK_LOCATION_CACHE
, please update the comment together
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.
sure
private static final IndexedSet<BlockLocation> BLOCK_LOCATION_CACHE = | ||
new IndexedSet<>(OBJECT_INDEX, WORKER_ID_INDEX); | ||
|
||
private static final Set<String> VALID_MEDIUM_TYPE_VALUES = |
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.
Athough MEM, SSD, HDD
is the default value of MASTER_TIERED_STORE_GLOBAL_MEDIUMTYPE
, but how to handle it when MASTER_TIERED_STORE_GLOBAL_MEDIUMTYPE
configured to other value?
How about get the set from MASTER_TIERED_STORE_GLOBAL_MEDIUMTYPE
?
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'll add a comment for this one
Preconditions.checkState(VALID_MEDIUM_TYPE_VALUES.contains(blockLocation.getTier()), | ||
"TierAlias must be one of {MEM, HDD and SSD} but got %s", | ||
blockLocation.getTier()); | ||
Preconditions.checkState(VALID_MEDIUM_TYPE_VALUES.contains(blockLocation.getMediumType()), |
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 supported mediumTypes are read from configuration too.
PropertyKey tierDirMediumConf =
PropertyKey.Template.WORKER_TIERED_STORE_LEVEL_DIRS_MEDIUMTYPE.format(mTierOrdinal);
List<String> dirMedium = Configuration.getList(tierDirMediumConf);
alluxio-bot, merge this please |
…memory Cache block location object to reduce memory consumption Number of possible Block location value is limited, up to 9 * number of workers. By caching it, memory consumption can be saved. N/A pr-link: Alluxio#16953 change-id: cid-9464ad2e70284b50d4c8a05dd5138f35e78bcb2d
Finish the todo task after Alluxio#16953 pr-link: Alluxio#17056 change-id: cid-e6ddc03b08e7a8a18cd53d77fa9a2f0edf4e1f57
What changes are proposed in this pull request?
Cache block location object to reduce memory consumption
Why are the changes needed?
Number of possible Block location value is limited, up to 9 * number of workers. By caching it, memory consumption can be saved.
Does this PR introduce any user facing changes?
N/A