-
Notifications
You must be signed in to change notification settings - Fork 210
Migrate google/btree to k8s util #336
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
Migrate google/btree to k8s util #336
Conversation
|
Welcome @pjsharath28! |
btree/btree_generic.go
Outdated
| // In Go 1.18 and beyond, a BTreeG generic is created, and BTree is a specific | ||
| // instantiation of that generic for the Item interface, with a backwards- | ||
| // compatible API. Before go1.18, generics are not supported, | ||
| // and BTree is just an implementation based around the Item interface. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t relevant and should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
btree/btree_generic.go
Outdated
| // There are two implementations; those suffixed with 'G' are generics, usable | ||
| // for any type, and require a passed-in "less" function to define their ordering. | ||
| // Those without this prefix are specific to the 'Item' interface, and use | ||
| // its 'Less' function for ordering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k/k only uses the generic implementation, do we need to keep both in utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally not needed, I removed the non-generic implementation
d458954 to
af10f52
Compare
|
/retest |
|
@pjsharath28: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
btree/btree.go
Outdated
| // BTree has its own FreeList, but multiple BTrees can share the same | ||
| // FreeList, in particular when they're created with Clone. | ||
| // Two Btrees using the same freelist are safe for concurrent write access. | ||
| type FreeListG[T any] struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we’re only keeping the generic variant, the G suffix isn’t necessary; please remove it throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
btree/btree.go
Outdated
| type ItemIterator[T any] func(item T) bool | ||
|
|
||
| // Ordered represents the set of types for which the '<' operator work. | ||
| type Ordered interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed this previously, sorry — please use cmp.Ordered instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks
I've resolved it as well
|
@pjsharath28 can you please move the code to see the existing code in there and the additional files that give it context (we have one in there forked from golang) |
|
/lgtm Thanks! |
|
please add LICENSE - https://github.com/google/btree/blob/master/LICENSE |
|
hmmm 68cb205 is not a license :) |
|
@pjsharath28 please squash and we are good to goo |
0a3a1ef to
aa383d1
Compare
|
/approve /hold @pohly please remove hold when happy :) |
Hmm, now there's only a single commit, but quite a bit was changed: I think it would be nicer if the first commit had just a verbatim copy of the upstream code that we want (i.e. removing files that we don't care about is fine) and the second commit contains our changes. |
aa383d1 to
f2dbb5e
Compare
|
The update has some comment changes, but those seem benign except that we now have "See http://godoc.org/github.com/google/btree for documentation." again. I think that should be removed in the second commit. The commits now allow reviewing the history of the code: 👍 |
f2dbb5e to
d87c826
Compare
|
/lgtm Based on #336 (comment) and only one minor diff compared to the already approved version. Not sure whether that's worth fixing? |
d87c826 to
cec797e
Compare
|
@pjsharath28: thanks for your patience! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, pjsharath28, pohly, skitt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The
github.com/google/btreelibrary used by k8s and etcd has been archived and is no longer maintained. This raises long-term concerns around security fixes and CVEs.We discussed multiple options to address this in the issue. Given that we only rely on a single source file from the library, we concluded that copying the code into
k8s.io/utilsis the simplest option. This avoids introducing or maintaining a separate fork while preserving identical behaviour.Which issue(s) this PR is related to:
Discussion issue: etcd-io/etcd#20991
Special notes for your reviewer:
Upstream Google attribution is preserved and the file header follows Kubernetes conventions while remaining Apache-2.0 compliant.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: