-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Linear] Ensure a resource is only serialized/hashed at most once to reply it #21
[Linear] Ensure a resource is only serialized/hashed at most once to reply it #21
Conversation
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.
LGTM
cacheVersion: cacheVersion, | ||
} | ||
} | ||
|
||
// getMarshaledResource lazily marshals the resource and returns the bytes. |
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.
When you say lazily you mean returns the marshalled resource right if it exists, otherwise it marshals it?
When I think of lazily returns i think of haskell style lazy returns.
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'm not familiar with Haskell, but here it is lazy computed within a getter.
In this case it's painfully returning an error, which can trigger if the user passes in a type not known from the protoregistry
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 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.
lgtm, just a suggestion to make things simpler.
pkg/cache/v3/linear.go
Outdated
// marshaledResource contains the marshaled version of the resource. | ||
// It is lazy initialized and should be accessed through getMarshaledResource | ||
marshaledResource []byte | ||
|
||
// mu is the mutex used to lazy compute the marshaled resource and stable version. | ||
mu sync.Mutex |
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 could be simplified quite a bit by using sync.Once
with a custom struct that ensures we compute the stable version and the serialized proto once:
[...]
// marshalResource computes the resource bytes and version...
marshalResource func() (ser, error)
}
In the initialization:
[...]
marshalResource: sync.OnceValues(func() (s ser, err error) {
s.res, err = MarshalResource(res)
h := sha256.New()
h.Write(c.res)
s.v = hex.EncodeToString(h.Sum(nil))
return c, err
}),
And then you can freely call marshalResource anywhere without having to worry about locking.
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.
Great suggestion, updated. In the future I'd like to support user provided too so it might become a bit more complex but should still be clearer than mutex-based
Merge activity
|
pkg/cache/v3/linear.go
Outdated
return c.stableVersion, nil | ||
// getMarshaledResource lazily marshals the resource and returns the bytes. | ||
func (c *cachedResource) getMarshaledResource() ([]byte, error) { | ||
return sync.OnceValues(func() ([]byte, error) { |
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'm not sure to understand how this effectively synchronizes the calls here. If you call the function returned by sync.OnceValues
, then different calls to getMarshaledResource
are not synchronized. AFAICT, the call to sync.OnceValues
is either unnecessary or not doing its job.
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.
Fixed. Missed the implementation part. I did a quick test to confirm it's now serialized only once
9e682c8
to
4ff6804
Compare
Currently the go-control-plane caches (both linear and snapshots) will serialize the resource as many time as there are clients receiving it.
This is an issue with control-planes watched by a lot of clients, especially with large resources (e.g. endpoints)
This PR ensures that the serialization occurs at most once per resource, in all cases (sotw/delta watches and linear/snapshot cache). A resource will still only be serialized if: