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

Improve retention mark files. #3706

Merged
merged 6 commits into from
May 18, 2021
Merged

Conversation

cyriltovena
Copy link
Contributor

This PR is two fold:

  1. Rotate mark files when they reached 100k marks, this is to ensure we max out marks file
    and don't create file that may be too big.

  2. Instead of inserting marks using the chunk id as the key, we insert using a natural sequence.
    This has two benefits:

  • Keys are ordered, and so insertion are faster.
  • Allows us to use boltdb fill percent to 100%, this means boltdb won't over allocate for inserting key in between unordered data.

Why ?

I realize that when inserting huge amount of marks, this operation can take up to hours since boltdb has to re-allocate pages over and over.
This is mainly because chunkid arrive without specific order.

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

@sandeepsukhani
Copy link
Contributor

sandeepsukhani commented May 10, 2021

This is mainly because chunkid arrive without specific order.

I might be wrong, but since boltdb iterates over keys in order, we should be getting chunk ids in order.

@cyriltovena
Copy link
Contributor Author

That's correct although I didn't wanted to rely on this, technically only the rotation and fill percent was required for the improvement. But I went ahead make that change just in case so we have this guarantee whatever the input.

@cyriltovena
Copy link
Contributor Author

Ping @sandeepsukhani should we revert the mark file system or are you ok with the approach ?

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

I just added a suggestion and a comment for a possible issue.

return err
}
binary.BigEndian.PutUint64(m.buf, id) // insert in order using sequence id.
if err := m.bucket.Put(m.buf, chunkID); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think reusing a buf for keys might cause a problem because the comment here says:
Supplied value must remain valid for the life of the transaction.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the key is cloned, but I'm thinking about the chunkId.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right going to keep the fillpercent but revert this change.

pkg/storage/stores/shipper/compactor/retention/marker.go Outdated Show resolved Hide resolved
cyriltovena and others added 4 commits May 18, 2021 09:27
This PR is two fold:

1) Rotate mark files when they reached 100k marks, this is to ensure we max out marks file
and don't create file that may be too big.

2) Instead of inserting marks using the chunk id as the key, we insert using a natural sequence.
This has two benefits:
- Keys are ordered, and so insertion are faster.
- Allows us to use boltdb fill percent to 100%, this means boltdb won't over allocate for inserting key in between unordered data.

Why ?

I realize that when inserting huge amount of marks, this operation can take up to hours since boltdb has to re-allocate pages over and over.
This is mainly because chunkid arrive without  specific order.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM!

@cyriltovena cyriltovena merged commit e1a3ab8 into grafana:main May 18, 2021
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.

None yet

2 participants