chia7712 commented on code in PR #844:
URL: https://github.com/apache/yunikorn-k8shim/pull/844#discussion_r1635416792
##########
pkg/client/clients.go:
##########
@@ -63,42 +92,96 @@ type Clients struct {
VolumeBinder volumebinding.SchedulerVolumeBinder
}
+func NewClients(scheduler api.SchedulerAPI, informerFactory
informers.SharedInformerFactory, configs *conf.SchedulerConf, kubeClient
KubeClient) *Clients {
+ // init informers
+ // volume informers are also used to get the Listers for the predicates
+ hasInformers := []hasInformer{}
+
+ podInformer := save(informerFactory.Core().V1().Pods(), &hasInformers)
+ nodeInformer := save(informerFactory.Core().V1().Nodes(), &hasInformers)
+ configMapInformer := save(informerFactory.Core().V1().ConfigMaps(),
&hasInformers)
+ storageInformer :=
save(informerFactory.Storage().V1().StorageClasses(), &hasInformers)
+ pvInformer := save(informerFactory.Core().V1().PersistentVolumes(),
&hasInformers)
+ pvcInformer :=
save(informerFactory.Core().V1().PersistentVolumeClaims(), &hasInformers)
+ priorityClassInformer :=
save(informerFactory.Scheduling().V1().PriorityClasses(), &hasInformers)
+ namespaceInformer := save(informerFactory.Core().V1().Namespaces(),
&hasInformers)
+ csiNodeInformer := save(informerFactory.Storage().V1().CSINodes(),
&hasInformers)
+ csiDriversInformer := save(informerFactory.Storage().V1().CSIDrivers(),
&hasInformers)
+ csiStorageCapacityInformer :=
save(informerFactory.Storage().V1().CSIStorageCapacities(), &hasInformers)
Review Comment:
> For example, even though this is more LOC, it's just much simpler:
yep, that is a good idea. However, the previous discussion
(https://github.com/apache/yunikorn-k8shim/pull/844#discussion_r1607656208)
seems to want a style of avoid ignoring the `SharedIndexInformer` in creating
`XXXInformer` . Otherwise, this simple way is good to me.
Let me summary the requests:
1. we must save the `SharedIndexInformer` from all used `XXXInformer`
(https://github.com/apache/yunikorn-k8shim/pull/844#discussion_r1607656208)
2. the `informerTypes` should be synced with all used `XXXInformer`
(https://github.com/apache/yunikorn-k8shim/pull/844#discussion_r1607656208)
I agree to @wilfred-s said "Not sure how we do that without manual work" -
Maybe we should let this PR go and focus on adding missed `CSINodeInformer`
and `NamespaceInformer` to `informerTypes` manually
--
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]
