chia7712 commented on code in PR #844:
URL: https://github.com/apache/yunikorn-k8shim/pull/844#discussion_r1607848267
##########
pkg/client/clients.go:
##########
@@ -67,38 +68,38 @@ func (c *Clients) GetConf() *conf.SchedulerConf {
return c.conf
}
+func (c *Clients) getInformers() []cache.SharedIndexInformer {
+ return []cache.SharedIndexInformer{
Review Comment:
> This list should be in sync with the informerTypes or the other way around.
> That was the issue that triggered YUNIKORN-2621
Not sure how we do that without manual work. The informers side is simple,
the listers are different for every type which makes it tricky.
agree to that is tricky. Maybe we can have a inner interface `HasInformer`
to offer informer, and then we cast it to true struct to create `Clients`. For
example:
```go
func (t Type) HasInformer(informerFactory informers.SharedInformerFactory)
HasInformer {
switch t {
case PVInformerHandlers:
return informerFactory.Core().V1().PersistentVolumes()
case PodInformerHandlers:
return informerFactory.Core().V1().Pods()
}
return nil
}
func NewAPIFactory(scheduler api.SchedulerAPI, informerFactory
informers.SharedInformerFactory, configs *conf.SchedulerConf, testMode bool)
*APIFactory {
kubeClient := NewKubeClient(configs.KubeConfig)
all := make(map[Type]HasInformer)
for s := range informerTypes {
all[Type(s)] = Type(s).HasInformer(informerFactory)
}
// init informers
// volume informers are also used to get the Listers for the predicates
nodeInformer := all[NodeInformerHandlers].(v1.NodeInformer)
podInformer := all[PodInformerHandlers].(v1.PodInformer)
}
```
With this change, we have to add new const `Type` when adding new informer
in the future, as all structs are from `Type(s).HasInformer`. Also, the map
`all` can pass to `Clients` to be used as "all informers". BTW, we can enable
`exhaustive` of `linters` to avoid missing statements in switch.
In short, we can consider adding following changes.
1. enable `exhaustive` check - avoid missing switch statement
2. `Type` can return a new inner interface `HasInformer` - bind the informer
to `Type`
3. cast `HasInformer` to true struct instead of accessing
`SharedInformerFactory` - make sure all used informers come from `Type`
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
