Update passing rate_limiter_priority for a PartitionedFilterBlockReader function to FS #10438
Conversation
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
gitbw95
changed the title
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -304,6 +305,7 @@ Status PartitionedFilterBlockReader::GetFilterPartitionBlock( | |||
| } | |||
|
|
|||
| ReadOptions read_options; | |||
| read_options.rate_limiter_priority = rate_limiter_priority; | |||
There was a problem hiding this comment.
I feel like the root cause of this issue is that GetFilterPartitionBlock() was pretending to have a proper ReadOptions. I think it would be better if we could plumb the operation's (e.g., Get()'s) ReadOptions down into GetFilterPartitionBlock() and use it here. Is it possible?
There was a problem hiding this comment.
In #10251 , we have passed the rate_limiter_priority from BlockBasedTable to MayMatch functions. I missed passing rate_limiter_priority into GetFilterPartitionBlock.
For ReadOptions param:
- I roughly remember that we just wanted to pass a light-weight rate_limiter_priority rather than ReadOptions into the filter functions.
- The filter functions may not need all the options from ReadOptions of the caller (e.g. Get()).
cc @pdillinger
There was a problem hiding this comment.
I agree it makes sense to refactor at some point to pass the operation's ReadOptions down to all the filter functions. But not all the options may be appropriate, so we eventually have to filter out the unnecessary ones before calling RetrieveBlock.
| @@ -304,6 +305,7 @@ Status PartitionedFilterBlockReader::GetFilterPartitionBlock( | |||
| } | |||
|
|
|||
| ReadOptions read_options; | |||
| read_options.rate_limiter_priority = rate_limiter_priority; | |||
There was a problem hiding this comment.
I agree it makes sense to refactor at some point to pass the operation's ReadOptions down to all the filter functions. But not all the options may be appropriate, so we eventually have to filter out the unnecessary ones before calling RetrieveBlock.
…er function to FS (facebook#10438) Summary: Add param rate_limiter_parameter in PartitionedFilterBlockReader::GetFilterPartitionBlock . Pull Request resolved: facebook#10438 Test Plan: Unit Tests. Reviewed By: anand1976 Differential Revision: D38266395 Pulled By: gitbw95 fbshipit-source-id: 3ed062a3b43d6df323371cb0d266f7fe869e9ad2
Summary:
Add param rate_limiter_parameter in PartitionedFilterBlockReader::GetFilterPartitionBlock .
Test Plan:
Unit Tests.