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.

Sign up for GitHub

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

Jump to bottom

feat: bump badger to v4 #14427

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: bump badger to v4 #14427

wants to merge 3 commits into from

Conversation

kruskall
Copy link
Member

Motivation/summary

badger v2 is ancient

use a separate folder for badgerv4 DB and drop old data

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Related issues

Closes #11546

@kruskall
feat: bump badger to v4
a697663 
use a separate folder for badgerv4 DB and drop old data
@kruskall kruskall requested a review from a team as a code owner October 23, 2024 12:05
@kruskall kruskall mentioned this pull request Oct 23, 2024
Open
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 23, 2024

This pull request does not have a backport label. Could you fix it @kruskall? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-8.x is the label to automatically backport to the 8.x branch.

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 23, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 23, 2024
marclop
marclop reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Can you add some tests for the new behavior?

x-pack/apm-server/sampling/processor_test.go Outdated Show resolved Hide resolved
x-pack/apm-server/main.go Outdated Show resolved Hide resolved
@kruskall
test: update storagegc test for badger v4
f15f6fd 
badger will write to the vlog only if the entry is greater than
ValueThreshold.
Update the test to use a larger span to write to the vlog
@kruskall
fix: remove old badger dir and add test
f04ad08
@kruskall
Copy link
Member Author

Change in behaviour:

  • old format is not compatible so we have to drop the v2 db. (the new database is stored in a separate folder)
  • [BREAKING]: Change how Badger handles WAL dgraph-io/badger#1555 which broken the gc test. Relevant part: With this PR, we no longer use value log as write-ahead log. Instead, each MemTable has its own WAL. Value logs now only write values which are greater than ValueThreshold, while MemTable WAL only writes smaller values and value pointers.

1pkg
1pkg reviewed Oct 24, 2024
View reviewed changes
x-pack/apm-server/main.go
@@ -117,6 +119,24 @@ func newTailSamplingProcessor(args beater.ServerParams) (*sampling.Processor, er
return nil, fmt.Errorf("failed to create Elasticsearch client for tail-sampling: %w", err)
}

oldStorageDir := paths.Resolve(paths.Data, oldTailSamplingStorageDir)
if entries, err := os.ReadDir(oldStorageDir); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to log the error if the syscall to open a directory has failed here? i.e. specifically checking for err != nil && err != io.EOF

marclop
marclop requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Can you add a changelog entry? This seems like the kind of change that needs a changelog entry.

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 30, 2024

This pull request is now in conflicts. Could you fix it @kruskall? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feat/badger-v4 upstream/feat/badger-v4
git merge upstream/main
git push upstream feat/badger-v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marclop marclop marclop requested changes

@1pkg 1pkg 1pkg approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
backport-8.x Automated backport to the 8.x branch with mergify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update badger to latest version
3 participants
@kruskall @marclop @1pkg