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.

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

[TechDebt] Minor improvement to #8428 (Account for dictionary-building buffer in global memory limit) #9032

Closed

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Oct 13, 2021

Summary/Context:

  • Renamed cache_rev_mng to compression_dict_buffer_cache_res_mgr
    • It is to distinguish with other potential cache_res_mgr in BlockBasedTableBuilder and to use correct short-hand for the words "reservation", "manager"
  • Added table_options.block_cache == nullptr in additional to table_options.no_block_cache == true to be conditions where we don't create a CacheReservationManager
    • Theoretically table_options.no_block_cache == true is equivalent to table_options.block_cache == nullptr by API. But since segment fault will be generated by passing nullptr into CacheReservationManager's constructor, it does not hurt to directly verify table_options.block_cache != nullptr before passing in
  • Renamed is_cache_full to exceeds_global_block_cache_limit
    • It is to hide implementation detail of cache reservation and to emphasize on the concept/design intent of caping memory within global block cache limit

Test:

  • Passing existing tests

@hx235 hx235 marked this pull request as draft October 13, 2021 23:53
@hx235 hx235 changed the title Minor improvement to #8428 [Draft] Minor improvement to #8428 Oct 13, 2021
@hx235 hx235 changed the title [Draft] Minor improvement to #8428 [Draft][Not Ready For Review] Minor improvement to #8428 Oct 20, 2021
@hx235 hx235 force-pushed the minor_improve_compress_dic_mem_account branch from 1838f5c to cb75b21 Compare October 24, 2021 04:40
@hx235 hx235 changed the title [Draft][Not Ready For Review] Minor improvement to #8428 Minor improvement to #8428 (Account for dictionary-building buffer in global memory limit) Oct 24, 2021
@hx235 hx235 force-pushed the minor_improve_compress_dic_mem_account branch from cb75b21 to 46d961b Compare October 24, 2021 04:45
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM

@hx235 hx235 marked this pull request as ready for review October 28, 2021 20:16
@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@hx235 merged this pull request in a5ec5e3.

@hx235 hx235 changed the title Minor improvement to #8428 (Account for dictionary-building buffer in global memory limit) [TechDebt] Minor improvement to #8428 (Account for dictionary-building buffer in global memory limit) Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants