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][ResourceMngmt]Minor improvement to CacheReservationManager/WriteBufferManager/CompressionDictBuilding #9139

Closed
wants to merge 3 commits into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Nov 5, 2021

Context:
Similar to #9072, #9032, this PR is to clear up some small tech debts in coding of my previous work.

Summary:

  • In CacheReservationManagerTest, replaced hard-coded test value related to CacheReservationManager::kSizeDummyEntry with CacheReservationManager::GetDummyEntrySize()
    • I did not replace other tests heavily relying on CacheReservationManager::kSizeDummyEntry being 256 * 1024. As we don't anticipate change in this value for the near future, we just live with it now.
  • In WriteBufferManager, renamed private variable cache_rev_mng/cache_rev_mng_mu to cache_res_mgr/cache_res_mgr_mu for accurate abbreviation of cache_reservation_manager
  • In BlockBasedTableBuilder, added a line of comment about releasing cache for releasing all the buffered data blocks used in compression dictionary building

Testing:

  • Relied on existing tests as the focus will be finding compilation error

@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.

@hx235
Copy link
Contributor Author

hx235 commented Nov 5, 2021

Update:

  • Ready for review when signals are cleared

@hx235 hx235 requested a review from jay-zhuang November 5, 2021 19:11
Copy link
Contributor

@zhichao-cao zhichao-cao 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
Copy link
Contributor Author

hx235 commented Nov 5, 2021

LGTM

Thanks for reviewing!!!!

@facebook-github-bot
Copy link
Contributor

@hx235 merged this pull request in 3018a3e.

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