-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: bump deps #166
chore: bump deps #166
Conversation
pkg/state/impl/etcd/etcd.go
Outdated
defer func() { | ||
clearbootstrapList() |
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.
I don't think I understand the goal of this change ?
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.
It's a leftover from my experimentations on removing the memory leak. Removed.
Direct deps: - run rekres - github.com/cosi-project/runtime to v0.9.3 - go.etcd.io/etcd/api/v3 to v3.5.18 - go.etcd.io/etcd/client/v3 to v3.5.18 - go.etcd.io/etcd/server/v3 to v3.5.18 - golang.org/x/sync to v0.11.0 - google.golang.org/grpc to v1.70.0 Indirect deps: - github.com/grpc-ecosystem/grpc-gateway/v2 to v2.26.0 - github.com/prometheus/common to v0.62.0 - github.com/siderolabs/protoenc to v0.2.2 - github.com/spf13/pflag to v1.0.6 - go.etcd.io/etcd/client/pkg/v3 to v3.5.18 - go.etcd.io/etcd/client/v2 to v2.305.18 - go.etcd.io/etcd/pkg/v3 to v3.5.18 - go.etcd.io/etcd/raft/v3 to v3.5.18 - go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc to v0.59.0 - go.opentelemetry.io/otel to v1.34.0 - go.opentelemetry.io/otel/exporters/otlp/otlptrace to v1.34.0 - go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc to v1.34.0 - go.opentelemetry.io/otel/metric to v1.34.0 - go.opentelemetry.io/otel/sdk to v1.34.0 - go.opentelemetry.io/otel/trace to v1.34.0 - golang.org/x/sys to v0.30.0 - golang.org/x/text to v0.22.0 - golang.org/x/time to v0.10.0 - google.golang.org/genproto to v0.0.0-20250204164813-702378808489 - google.golang.org/genproto/googleapis/api to v0.0.0-20250204164813-702378808489 - google.golang.org/genproto/googleapis/rpc to v0.0.0-20250204164813-702378808489 - google.golang.org/protobuf to v1.36.4 Signed-off-by: Dmitriy Matrenichev <[email protected]>
go.uber.org/goleak v1.3.0 | ||
go.uber.org/zap v1.27.0 | ||
golang.org/x/sync v0.10.0 | ||
google.golang.org/grpc v1.69.4 |
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.
out of my curiousity, what is the reason to bump so much aggressively? this prevents using easily lower versions if these versions are buggy for whatever reason in Omni.
Does this library work any better with new grpc
package?
I can understand bumping etcd
, as it's clearly the dependency here and the core thing of state-etcd
, but why protobuf? Same question for COSI runtime itself?
Why is the fix buried inside chore: bump deps
?
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.
out of my curiousity, what is the reason to bump so much aggressively? this prevents using easily lower versions if these versions are buggy for whatever reason in Omni.
So, it's mostly a hypothesis but it comes from the suggestion that since omni
imports so many libraries directly or indirectly, some of those breakages can be caught during lint-test
run in our libraries before we get them into Omni. This also also "splits" required work.
I'm coming from this thought:
omni
imports state-etcd
which imports a lot of third party libraries (Set A
). Some of this Set A
are also direct imports for Omni (Set B ∩ Set A
) or direct imports of omni dependencies (Set C ∩ Set A
). Going on updates aggressively means (updating Set A
) means we can always revert those changes/fix those before we are forced to update to those dependencies because our direct imports with fixes that we need updated those imports themselves.
Another thought, is that while some of our 3rd party modules tend to stay updated, others do not. That creates complicated situation when, for example updating Set A
breaks Set B
. This should not happen but in practice in happened before.
But this also creates unpleasant complication that in bad case scenario we have to revert our module which is imported by omni
if Omni breaks on production, and we will have to revert this change. I don't have a good answer for that tho.
Same question for COSI runtime itself?
We broke our code transitively before when our updates to COSI runtime
broke omni
. Since state-etcd
is a part of call-chain between omni and a COSI runtime
, I think it makes sense to update runtime here before omni
.
Why is the fix buried inside chore: bump deps?
There is no fix in this PR, I think?
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.
There were changes that changed the way the bootstrap list is cleared.
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.
There were changes that changed the way the bootstrap list is cleared.
Ah, I removed those. During experiments I assumed that if we return
before we reach bootstrapList = nil
this will prevent GC from collecting bootstrapList
while we are executing defer
s which can be arbitrary long. But it doesn't look like the case: GC is smart enough to figure out those are no longer used. Actually I'm not sure that even bootstrapList = nil
does anything useful at this point.
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.
It's a tough question about dependencies, but it's also true that libraries should allow some flexibility for consumers on picking up the version they want, as if libraries pull latest and greatest, there is no flexibility whatsoever. So I guess both points are valid (yours and mine), and I'm not sure which one is the best approach.
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.
Let's merge this one, and probably figure our strategy on updating stuff (Planning? Separate meeting?) and set it in paper. I think we need one anyway, because right now updating stuff relies mostly on intuitive desicion rather than rational one.
/m |
Direct deps:
Indirect deps: