xdsclient: update watcher API as per gRFC A88 #7977
Conversation
purnesh42H
added
Type: Feature
purnesh42H
force-pushed
the
a88-watcher-api
branch
7 times, most recently
from
8d198b7 to
a9b45c0
Compare
purnesh42H
force-pushed
the
a88-watcher-api
branch
2 times, most recently
from
4009e3e to
57dbf23
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7977 +/- ##
==========================================
- Coverage 82.28% 82.14% -0.15%
==========================================
Files 381 381
Lines 38539 38558 +19
==========================================
- Hits 31712 31672 -40
- Misses 5535 5583 +48
- Partials 1292 1303 +11
|
purnesh42H
force-pushed
the
a88-watcher-api
branch
4 times, most recently
from
43c9adb to
89f475a
Compare
purnesh42H
force-pushed
the
a88-watcher-api
branch
from
89f475a to
a48b5bb
Compare
| // ResourceWatcher is an interface that can to be implemented to wrap the | ||
| // callbacks to be invoked for different events corresponding to the resource | ||
| // being watched. |
There was a problem hiding this comment.
I don't feel like this change to the comment adds much value. The fact that it is an interface is clear from the type definition and since it is an interface, it *needs *to be implemented. So, I would leave this unchanged or improve it in a different way.
| // OnUpdate is invoked to report an update for the resource being watched. | ||
| // OnResourceChanged is invoked to notify the watcher of a new version of | ||
| // the resource received from the xDS server or an error indicating the | ||
| // reason why the resource cannot be obtained. |
| // type for the resource being watched. In case of error, the ResourceData | ||
| // is nil otherwise its not nil and error is nil but both will never be nil | ||
| // together. |
There was a problem hiding this comment.
This is super confusing.
Should we consider implementing a naive version of the C++ StatusOr?
It could be something like
type ResourceDataOrError {
data ResourceData
err error
}
func (r *ResourceDataOrError) ResourceData {
return r.data
}
func (r *ResourceDataOrError) Error {
return r.err
}
func NewResourceDataOrErrorFromResourceData(data ResourceData) *ResourceDataOrError {
return &ResourceDataOrError{data: data}
}
func NewResourceDataOrErrorFromError(err error) *ResourceDataOrError {
return &ResourceDataOrError{err: err}
}The fact that the fields are unexported means that the above struct can only be constructed with one of the above NewXxx, and that guarantees that only one of the two fields will be set at any given point in time.
The names of the NewXxx functions suck at this point. But maybe with some thought, we can come up with something better.
Or we could event come up with a generic version of this which takes inspiration from the Rust Result<T, E> type.
@dfawley : thoughts?
There was a problem hiding this comment.
I think the Go-typical way to implement a sum type is to just have two fields, only one of which is set. And a comment saying as much.
// Whatever contains either Data or Err if an error occurred while trying to obtain Data.
type Whatever struct {
Err error
Data Something
}That effectively mirrors:
func SomethingThatCouldFail() (Something, error) {}There was a problem hiding this comment.
Updated to
// ResourceDataOrError is a struct that contains either ResourceData or error.
// It is used to represent the result of an xDS resource update. Exactly one of
// Data or Err will be non-nil.
type ResourceDataOrError struct {
Data ResourceData
Err error
}
| // will never be nil together. | ||
| // | ||
| // Watcher is expected to use the most recent value passed to | ||
| // OnResourceChanged(), regardless of whether that's a resource or an error |
There was a problem hiding this comment.
Nit: s/ whether that's a resource/whether that's a ResourceData/
| // OnResourceChanged is invoked to notify the watcher of a new version of | ||
| // the resource received from the xDS server or an error indicating the | ||
| // reason why the resource could not be obtained. | ||
| // | ||
| // The ResourceData of the ResourceDataOrError needs to be type asserted to | ||
| // the appropriate type for the resource being watched. In case of error, | ||
| // the ResourceData is nil otherwise its not nil and error is nil but both | ||
| // will never be nil together. |
There was a problem hiding this comment.
How about replacing the first two paragraphs with the following:
// OnResourceChanged is invoked to notify the watcher of a new version of
// the resource received from the xDS server or an error indicating the
// reason why the resource could not be obtained.
//
// In the former case, this callback will be invoked with a non-nil ResourceData
// in ResourceDataOrError. The ResourceData of the ResourceDataOrError needs
// to be type asserted to the appropriate type for the resource being watched.
//
// In the latter case, this callback will be invoked with a non-nil error value in ResourceDataOrError.
| // - connection failure (if resource is not cached) | ||
| OnResourceChanged(*ResourceDataOrError, OnDoneFunc) | ||
|
|
||
| // If resource is already cached, it is invoked under different error |
There was a problem hiding this comment.
Please start with the docstring with the name of the function, as is customary in Go.
| // - connection failure (if resource is not cached) | ||
| OnResourceChanged(*ResourceDataOrError, OnDoneFunc) | ||
|
|
||
| // If resource is already cached, it is invoked under different error |
There was a problem hiding this comment.
Please start with the docstring with the name of the function, as is customary in Go.
| // - connection failure (if resource is not cached) | ||
| OnResourceChanged(*ResourceDataOrError, OnDoneFunc) | ||
|
|
||
| // If resource is already cached, it is invoked under different error |
There was a problem hiding this comment.
Please start with the docstring with the name of the function, as is customary in Go.
| if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound { | ||
| r.logger.Infof("Received resource-not-found-error for Listener resource %q", r.ldsResourceName) | ||
| } else { | ||
| r.logger.Infof("Received on-resource-changed error for Listener resource %q: %v", r.ldsResourceName, err) | ||
| } |
There was a problem hiding this comment.
How about simply printing the error in the log instead of checking for the error type and printing different log statements. Printing the error value itself should give information about what type of error was recieved.
| if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound { | ||
| r.logger.Infof("Received resource-not-found-error for RouteConfiguration resource %q", name) | ||
| } else { | ||
| r.logger.Infof("Received on-resource-changed error for RouteConfiguration resource %q: %v", name, err) | ||
| } |
| if xdsresource.ErrType(update.Err) == xdsresource.ErrorTypeResourceNotFound { | ||
| er.logger.Infof("EDS discovery mechanism for resource %q reported resource-does-not-exist error", er.nameToWatch) | ||
| } else { | ||
| er.logger.Infof("EDS discovery mechanism for resource %q reported on resource changed error: %v", er.nameToWatch, update.Err) | ||
| } |
There was a problem hiding this comment.
Same here. Please print the error to logs instead of having to check for the type of error.
| @@ -525,15 +525,14 @@ func (b *cdsBalancer) onClusterError(name string, err error) { | |||
| // TRANSIENT_FAILURE. | |||
| // | |||
| // Only executed in the context of a serializer callback. | |||
| func (b *cdsBalancer) onClusterResourceNotFound(name string) { | |||
| err := xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "resource name %q of type Cluster not found in received response", name) | |||
| func (b *cdsBalancer) onClusterResourceChangedError(name string, err error) { | |||
| if b.childLB != nil { | ||
| b.childLB.ResolverError(err) | ||
| } else { | ||
| // If child balancer was never created, fail the RPCs with errors. | ||
| b.ccw.UpdateState(balancer.State{ | ||
| ConnectivityState: connectivity.TransientFailure, | ||
| Picker: base.NewErrPicker(err), | ||
| Picker: base.NewErrPicker(fmt.Errorf("%q: %v", name, err)), |
There was a problem hiding this comment.
Have you verified that this error is as descriptive as what we had earlier?
This is the first part of implementing gRFC A88 (grpc/proposal#466).
This introduces the new watcher API but does not change any of the existing behavior. This table summarizes the API changes and behavior for each case:
OnResourceDoesNotExist()OnResourceChanged(NOT_FOUND)OnResourceDoesNotExist()OnResourceChanged(NOT_FOUND)OnError()OnResourceChanged(status)if resource NOT already cached;OnAmbientError(status)otherwiseOnError()OnResourceChanged(status)if resource NOT already cached;OnAmbientError(status)otherwiseOnError()OnResourceChanged(status)if resource NOT already cached;OnAmbientError(status)otherwiseOnUpdate(resource)OnResourceChanged(resource)RELEASE NOTES: