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

Enhanced bytes limiter with data type param #7414

Merged
merged 17 commits into from
Jun 13, 2024

Conversation

justinjung04
Copy link
Contributor

@justinjung04 justinjung04 commented Jun 4, 2024

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Introducing two main changes to enhance bytes-based limiting:

  1. Changed BytesLimiter.Reserve(num uint64) err to BytesLimiter.ReserveWithType(num uint64, dataType storeDataType) err such that the bytes limiter logic to be more flexible. This is useful given that processing of postings, series and chunks have different impact to pod resource usage. For example, user can have a bytes limiter that has different weights for different data types.
  2. nit: Created queryStats.add func to update query stats info more consistently

Verification

Added unit test and e2e test.

@justinjung04 justinjung04 changed the title Enhanced bytes limiter Enhanced bytes limiter with data type param Jun 4, 2024
@justinjung04 justinjung04 marked this pull request as ready for review June 4, 2024 23:00
@justinjung04 justinjung04 force-pushed the enhanced-bytes-limiter branch from f910fdc to 6b0040d Compare June 5, 2024 02:42
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/limiter.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
@justinjung04 justinjung04 force-pushed the enhanced-bytes-limiter branch from 74d2267 to 93136a7 Compare June 11, 2024 16:41
Signed-off-by: Justin Jung <[email protected]>
yeya24
yeya24 previously approved these changes Jun 11, 2024
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM. Just a small nit on the changelog

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@harry671003 harry671003 left a comment

Choose a reason for hiding this comment

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

Great work Justin!

Signed-off-by: Justin Jung <[email protected]>
@yeya24
Copy link
Contributor

yeya24 commented Jun 12, 2024

Maybe @fpetkovski @MichaHoffmann or @saswatamcode can help take a look? Thanks

@yeya24
Copy link
Contributor

yeya24 commented Jun 13, 2024

I will merge this change tomorrow if there is no objection or new review comments. Since this change is not user facing at all, we can address in future PRs if you have any other comments

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks, generally LGTM! I think it can be put in changelog as your Store error will now tell you the type of data that exceeded bytes limit?

Also any plans to expose these to user somehow?

@justinjung04
Copy link
Contributor Author

justinjung04 commented Jun 13, 2024

Thanks for taking a look @saswatamcode !

I think it can be put in changelog as your Store error will now tell you the type of data that exceeded bytes limit?

I haven't changed the error message returned to users, as they already contained the data type information. The main goal of this change was to enhance the limiter logic with access to the data type, which can be hidden from the users.

Also any plans to expose these to user somehow?

Yes! We have been noticing that resource consumption (CPU and memory) for postings, series and chunks are quite different. For example, we were able to fetch up to 1GB/s of chunks until the CPU is exhausted, while we were only able to fetch 258MB/s of series. We should definitely update the limiter logic to assign different weights per data type.

@yeya24 yeya24 merged commit 651a4a4 into thanos-io:main Jun 13, 2024
20 checks passed
hczhu-db pushed a commit to databricks/thanos that referenced this pull request Aug 22, 2024
* Refactor existing stats incrementation for touched and fetched data

Signed-off-by: Justin Jung <[email protected]>

* Add TypedBytesLimiter

Signed-off-by: Justin Jung <[email protected]>

* Remove addAndCheck func

Signed-off-by: Justin Jung <[email protected]>

* Update BytesLimiter interface to accept dataType param

Signed-off-by: Justin Jung <[email protected]>

* Added tests

Signed-off-by: Justin Jung <[email protected]>

* Fix build + changelog

Signed-off-by: Justin Jung <[email protected]>

* Fix wrong data type

Signed-off-by: Justin Jung <[email protected]>

* Changed storeDataType to be exported

Signed-off-by: Justin Jung <[email protected]>

* Revert []BytesLimiter to BytesLimtier

Signed-off-by: Justin Jung <[email protected]>

* Lint

Signed-off-by: Justin Jung <[email protected]>

* More reverts

Signed-off-by: Justin Jung <[email protected]>

* More

Signed-off-by: Justin Jung <[email protected]>

* Rename DefaultBytesLimiterFactory back to NewBytesLimiterFactory

Signed-off-by: Justin Jung <[email protected]>

* Changed StoreDataType from string to int

Signed-off-by: Justin Jung <[email protected]>

* Removed nil check for bytesLimiter

Signed-off-by: Justin Jung <[email protected]>

* nit

Signed-off-by: Justin Jung <[email protected]>

* Removed changelog

Signed-off-by: Justin Jung <[email protected]>

---------

Signed-off-by: Justin Jung <[email protected]>
hczhu-db pushed a commit to databricks/thanos that referenced this pull request Aug 22, 2024
* Refactor existing stats incrementation for touched and fetched data

Signed-off-by: Justin Jung <[email protected]>

* Add TypedBytesLimiter

Signed-off-by: Justin Jung <[email protected]>

* Remove addAndCheck func

Signed-off-by: Justin Jung <[email protected]>

* Update BytesLimiter interface to accept dataType param

Signed-off-by: Justin Jung <[email protected]>

* Added tests

Signed-off-by: Justin Jung <[email protected]>

* Fix build + changelog

Signed-off-by: Justin Jung <[email protected]>

* Fix wrong data type

Signed-off-by: Justin Jung <[email protected]>

* Changed storeDataType to be exported

Signed-off-by: Justin Jung <[email protected]>

* Revert []BytesLimiter to BytesLimtier

Signed-off-by: Justin Jung <[email protected]>

* Lint

Signed-off-by: Justin Jung <[email protected]>

* More reverts

Signed-off-by: Justin Jung <[email protected]>

* More

Signed-off-by: Justin Jung <[email protected]>

* Rename DefaultBytesLimiterFactory back to NewBytesLimiterFactory

Signed-off-by: Justin Jung <[email protected]>

* Changed StoreDataType from string to int

Signed-off-by: Justin Jung <[email protected]>

* Removed nil check for bytesLimiter

Signed-off-by: Justin Jung <[email protected]>

* nit

Signed-off-by: Justin Jung <[email protected]>

* Removed changelog

Signed-off-by: Justin Jung <[email protected]>

---------

Signed-off-by: Justin Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants