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

feat: bump badger to v4 #40939

Merged
merged 13 commits into from
Sep 28, 2024
Merged

feat: bump badger to v4 #40939

merged 13 commits into from
Sep 28, 2024

Conversation

kruskall
Copy link
Member

Proposed commit message

bump badger to current major version
drop ristretto replace directive as the glog dependency was removed upstream
See dgraph-io/ristretto#350

bonus point for dropping other unused indirect dependencies

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

bump badger to current major version
drop ristretto replace directive as the glog dependency
was removed upstream
@kruskall kruskall requested review from a team as code owners September 21, 2024 14:04
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 21, 2024
Copy link
Contributor

mergify bot commented Sep 21, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kruskall? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

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

Copy link
Contributor

mergify bot commented Sep 21, 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 and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 21, 2024
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Sep 23, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 23, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@mauri870
Copy link
Member

For context, there seems to be no breaking changes in this migration: https://github.com/dgraph-io/badger/releases/tag/v4.0.0.

@blakerouse
Copy link
Contributor

@kruskall Could you remove the replace on github.com/golang/glog? This is no longer needed as the upgraded badger doesn't pull in that dependency anymore.

@kruskall
Copy link
Member Author

@blakerouse that's true but unfortunately google.golang.org/grpc is still pulling in glog :(

@blakerouse
Copy link
Contributor

@kruskall Really? I could have sworn when I looked at the glog dependency it was only being used because of badger v3.

@blakerouse
Copy link
Contributor

We also pull in google.golang.org/grpc in elastic-agent and it doesn't pull in github.com/golang/glog. See here: https://github.com/elastic/elastic-agent/blob/main/go.mod#L72 (and no glog present)

@kruskall
Copy link
Member Author

https://github.com/grpc/grpc-go/blob/941102b7811f4431de807b36d1caee9f2ff5f614/go.mod#L9 :(

I guess this is safe because we're lucky nobody imports the glogger package in grpc-go so the compiler is being smart and trimming the package and the transitive dependencies.

Thanks for the review! 🙇

@kruskall
Copy link
Member Author

verifying github.com/dgraph-io/badger/[email protected]: checksum mismatch
	downloaded: h1:JZ8tapVYg+6sFQqg+BOokAxX+t09UeVPsPoXIq7jlNg=
	go.sum:     h1:lcsCE1/1qrRhqP+zYx6xDZb8n7U+QlwNicpc676Ub40=
SECURITY ERROR
This download does NOT match an earlier download recorded in go.sum.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

This is fun 🙂

@kruskall
Copy link
Member Author

In case people bump into this and are concerned:

github v4.3.0 tag (https://github.com/dgraph-io/badger/releases/tag/v4.3.0) points at dgraph-io/badger@02d7531

go list -m -json github.com/dgraph-io/badger/[email protected] points at dgraph-io/badger@2725dc8

The diff between the two commits looks safe: dgraph-io/badger@2725dc8...02d7531

I'll revert to v4.2.0 in any case, rewriting tags is concerning and should never be attempted with go modules and immutable versions.

@kruskall kruskall merged commit 4fb90ac into elastic:main Sep 28, 2024
141 checks passed
@kruskall kruskall deleted the feat/bump-badger branch September 28, 2024 10:24
mergify bot pushed a commit that referenced this pull request Sep 28, 2024
* feat: bump badger to v4

bump badger to current major version
drop ristretto replace directive as the glog dependency
was removed upstream

* lint: fix linter issues

* feat: drop glog replace directive

* feat: downgrade to badger v4.2.0 as the latest version was tampered

* lint: update notice file

* look at what they made me do :(

(cherry picked from commit 4fb90ac)
pierrehilbert pushed a commit that referenced this pull request Sep 29, 2024
* feat: bump badger to v4

bump badger to current major version
drop ristretto replace directive as the glog dependency
was removed upstream

* lint: fix linter issues

* feat: drop glog replace directive

* feat: downgrade to badger v4.2.0 as the latest version was tampered

* lint: update notice file

* look at what they made me do :(

(cherry picked from commit 4fb90ac)

Co-authored-by: kruskall <[email protected]>
@carsonip
Copy link
Member

carsonip commented Oct 8, 2024

Is the team aware of the implication of upgrading badger across a major version? According to badger README,

New major versions are released when the data format on disk changes in an incompatible way.
v2.x.x and v3.x.x are data incompatible as their major version implies, so files created with v2.x.x will need to be converted into the new format before they can be used by v3.0.0.

I see that beats use badger for a persistent cache. If this cache lives across version upgrade without proper handling, there may be a problem.

@blakerouse
Copy link
Contributor

@carsonip badger/v4 has no on-disk changes so it can be upgraded safely with no issues - dgraph-io/badger@v3.2103.5...v4.0.0#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR10

@carsonip
Copy link
Member

carsonip commented Oct 8, 2024

Thanks, sorry I missed that statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants