You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/01/19 05:55:39 UTC
[GitHub] [apisix-dashboard] imjoey opened a new issue #1331: feat: Better design for the Create and Update methods of Store interface
imjoey opened a new issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331
# Feature request
## Please describe your feature
This issue is originated from discussions with @ShiningRush in #1322 , current definition of `Update` methods in Store interface `Update(ctx context.Context, obj interface{}, createIfNotExist bool) (interface{}, error)` should be changed to `Update(ctx context.Context, obj interface{}, createIfNotExist bool) error`. The reason is that the input parameter `obj` already holds the pointer of data that to send back to response, so there's no need to explicitly return it via the returned parameter `interface{}`.
## Additional context
Current definition of `Create` methods is `Create(ctx context.Context, obj interface{}) (interface{}, error)`, shall we also refactor it to `Create(ctx context.Context, obj interface{}) error`? Looking forward to your insights @ShiningRush , much appreciated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey commented on issue #1331: feat: Better design for the Create and Update methods of Store interface
Posted by GitBox <gi...@apache.org>.
imjoey commented on issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331#issuecomment-764290323
@ShiningRush @starsz
I refer to the implementations of the `rest.Store` and `Storage` in Kubernetes APIServer. From the following code snippet (refer to https://github.com/kubernetes/apiserver/blob/5c8a24e0c50e5392e4766e59b41732074cd303c6/pkg/storage/interfaces.go#L214-L245 .), I get that the `Update` interface of `Storage` does not have returned data, just as the `Storage` interface we defined in apisix-dashboard.
```go
// GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'ptrToType')
// retrying the update until success if there is index conflict.
// Note that object passed to tryUpdate may change across invocations of tryUpdate() if
// other writers are simultaneously updating it, so tryUpdate() needs to take into account
// the current contents of the object when deciding how the update object should look.
// If the key doesn't exist, it will return NotFound storage error if ignoreNotFound=false
// or zero value in 'ptrToType' parameter otherwise.
// If the object to update has the same value as previous, it won't do any update
// but will return the object in 'ptrToType' parameter.
// If 'cachedExistingObject' is non-nil, it can be used as a suggestion about the
// current version of the object to avoid read operation from storage to get it.
// However, the implementations have to retry in case suggestion is stale.
//
// Example:
//
// s := /* implementation of Interface */
// err := s.GuaranteedUpdate(
// "myKey", &MyType{}, true,
// func(input runtime.Object, res ResponseMeta) (runtime.Object, *uint64, error) {
// // Before each invocation of the user defined function, "input" is reset to
// // current contents for "myKey" in database.
// curr := input.(*MyType) // Guaranteed to succeed.
//
// // Make the modification
// curr.Counter++
//
// // Return the modified object - return an error to stop iterating. Return
// // a uint64 to alter the TTL on the object, or nil to keep it the same value.
// return cur, nil, nil
// },
// )
GuaranteedUpdate(
ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool,
precondtions *Preconditions, tryUpdate UpdateFunc, cachedExistingObject runtime.Object) error
```
While regarding to the `Store`, we could see it does support returned the new value for the `Update` interface, shown as below ( refer to https://github.com/kubernetes/kubernetes/blob/a04b6e4b1671810ede5b8cacf4527741781d6fb9/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go#L276) .
```go
// StandardStorage is an interface covering the common verbs. Provided for testing whether a
// resource satisfies the normal storage methods. Use Storage when passing opaque storage objects.
type StandardStorage interface {
Getter
Lister
CreaterUpdater
GracefulDeleter
CollectionDeleter
Watcher
}
// Updater is an object that can update an instance of a RESTful object.
type Updater interface {
// New returns an empty object that can be used with Update after request data has been put into it.
// This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
New() runtime.Object
// Update finds a resource in the storage and updates it. Some implementations
// may allow updates creates the object - they should set the created boolean
// to true.
Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}
// CreaterUpdater is a storage object that must support both create and update.
// Go prevents embedded interfaces that implement the same method.
type CreaterUpdater interface {
Creater
Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}
```
So shall we keep the design the same as kubernetes for `Update` interface of `Store` ? Very looking forward to your insights. @starsz @ShiningRush .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey closed issue #1331: feat: Better design for the Create and Update methods of Store interface
Posted by GitBox <gi...@apache.org>.
imjoey closed issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey commented on issue #1331: feat: Better design for the Create and Update methods of Store interface
Posted by GitBox <gi...@apache.org>.
imjoey commented on issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331#issuecomment-764290323
@ShiningRush @starsz
I refer to the implementations of the `rest.Store` and `Storage` in Kubernetes APIServer. From the following code snippet (refer to https://github.com/kubernetes/apiserver/blob/5c8a24e0c50e5392e4766e59b41732074cd303c6/pkg/storage/interfaces.go#L214-L245 .), I get that the `Update` interface of `Storage` does not have returned data, just as the `Storage` interface we defined in apisix-dashboard.
```go
// GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'ptrToType')
// retrying the update until success if there is index conflict.
// Note that object passed to tryUpdate may change across invocations of tryUpdate() if
// other writers are simultaneously updating it, so tryUpdate() needs to take into account
// the current contents of the object when deciding how the update object should look.
// If the key doesn't exist, it will return NotFound storage error if ignoreNotFound=false
// or zero value in 'ptrToType' parameter otherwise.
// If the object to update has the same value as previous, it won't do any update
// but will return the object in 'ptrToType' parameter.
// If 'cachedExistingObject' is non-nil, it can be used as a suggestion about the
// current version of the object to avoid read operation from storage to get it.
// However, the implementations have to retry in case suggestion is stale.
//
// Example:
//
// s := /* implementation of Interface */
// err := s.GuaranteedUpdate(
// "myKey", &MyType{}, true,
// func(input runtime.Object, res ResponseMeta) (runtime.Object, *uint64, error) {
// // Before each invocation of the user defined function, "input" is reset to
// // current contents for "myKey" in database.
// curr := input.(*MyType) // Guaranteed to succeed.
//
// // Make the modification
// curr.Counter++
//
// // Return the modified object - return an error to stop iterating. Return
// // a uint64 to alter the TTL on the object, or nil to keep it the same value.
// return cur, nil, nil
// },
// )
GuaranteedUpdate(
ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool,
precondtions *Preconditions, tryUpdate UpdateFunc, cachedExistingObject runtime.Object) error
```
While regarding to the `Store`, we could see it does support returned the new value for the `Update` interface, shown as below ( refer to https://github.com/kubernetes/kubernetes/blob/a04b6e4b1671810ede5b8cacf4527741781d6fb9/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go#L276) .
```go
// StandardStorage is an interface covering the common verbs. Provided for testing whether a
// resource satisfies the normal storage methods. Use Storage when passing opaque storage objects.
type StandardStorage interface {
Getter
Lister
CreaterUpdater
GracefulDeleter
CollectionDeleter
Watcher
}
// Updater is an object that can update an instance of a RESTful object.
type Updater interface {
// New returns an empty object that can be used with Update after request data has been put into it.
// This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
New() runtime.Object
// Update finds a resource in the storage and updates it. Some implementations
// may allow updates creates the object - they should set the created boolean
// to true.
Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}
// CreaterUpdater is a storage object that must support both create and update.
// Go prevents embedded interfaces that implement the same method.
type CreaterUpdater interface {
Creater
Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}
```
So shall we keep the design the same as kubernetes for `Update` interface of `Store` ? Very looking forward to your insights. @starsz @ShiningRush .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] imjoey commented on issue #1331: feat: Better design for the Create and Update methods of Store interface
Posted by GitBox <gi...@apache.org>.
imjoey commented on issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331#issuecomment-770523027
@juzhiyuan @starsz @ShiningRush
Since not get more opinions for some time, I would prefer to close this issue for now, please feel free to reopen for in-depth discussions if anything still unclear. Thank you all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on issue #1331: feat: Better design for the Create and Update methods of Store interface
Posted by GitBox <gi...@apache.org>.
starsz commented on issue #1331:
URL: https://github.com/apache/apisix-dashboard/issues/1331#issuecomment-763539826
> The reason is that the input parameter obj already holds the pointer of data that to send back to response, so there's no need to explicitly return it via the returned parameter interface{}.
Right, there is no need.
But I think we can still keep the logic now. If someday, we want to do some custom design in GenericStore, e.g. inject some filed in the returned parameter. So the returned parameter value is different from the parameter.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org