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

[ResourceMngmt] Add new API CacheReservationManager::GetDummyEntrySize() #9072

Closed
wants to merge 2 commits into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Oct 24, 2021

Note: it might conflict with another CRM related PR #9071 and so will merge after that's merged.

Context:
As CacheReservationManager being used by more memory users, it is convenient to retrieve the dummy entry size for CacheReservationManager instead of hard-coding 256 * 1024 in writing tests. Plus it allows more flexibility to change our implementation on dummy entry size.

A follow-up PR is needed to replace those hard-coded dummy entry size value in db_test2.cc, db_write_buffer_manager_test.cc, write_buffer_manager_test.cc, table_test.cc and the ones introduced in #9072 (comment).

Summary:

  • Exposed the private static constexpr kDummyEntrySize through public static CacheReservationManager::GetDummyEntrySize()

Test:

  • Passing new tests
  • Passing existing tests

@hx235 hx235 marked this pull request as draft October 24, 2021 05:09
@hx235 hx235 force-pushed the crm_get_dummy_entry_size branch 2 times, most recently from e47e874 to 03c7e27 Compare October 28, 2021 21:48
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 30, 2021 00:38
@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 hx235 force-pushed the crm_get_dummy_entry_size branch from 03c7e27 to ed85f45 Compare November 1, 2021 18:25
@facebook-github-bot
Copy link
Contributor

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

@hx235
Copy link
Contributor Author

hx235 commented Nov 1, 2021

Update:

  • Add new line EOF as required

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

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.

3 participants