The method io.kubernetes.client.informer.SharedInformerFactory#sharedIndexInformerFor should ideally be idempotent.

[Sud0x67]

Describe the bug
In our service, we utilize the SharedInformerFactory to generate informers. However, we encountered unexpected behavior when invoking it multiple times. Specifically, it returns a new informer for the same apiTypeClass without maintaining it within the internal map named informers. Consequently, when we call the io.kubernetes.client.informer.SharedInformerFactory#startAllRegisteredInformers method, our informer doesn't start as anticipated. The code is as follows:

  public synchronized <ApiType extends KubernetesObject, ApiListType extends KubernetesListObject>
      SharedIndexInformer<ApiType> sharedIndexInformerFor(
          ListerWatcher<ApiType, ApiListType> listerWatcher,
          Class<ApiType> apiTypeClass,
          long resyncPeriodInMillis,
          BiConsumer<Class<ApiType>, Throwable> exceptionHandler) {

    SharedIndexInformer<ApiType> informer =
        new DefaultSharedIndexInformer<>(
            apiTypeClass, listerWatcher, resyncPeriodInMillis, new Cache<>(), exceptionHandler);
    this.informers.putIfAbsent(TypeToken.get(apiTypeClass).getType(), informer);
    return informer;
  }

This is a bit confusing. I don't understand why the factory created it but doesn't keep it. Could someone explain the purpose of this design to me?

Describe the solution you'd like

I believe the factory should retain all the informers it creates, ensuring that when we call startAllRegisteredInformers, the returned informer starts correctly. Alternatively, the method SharedInformerFactory#sharedIndexInformerFor should be idempotent. This means that if we create an informer for the same apiClassType, it should return the initial instance, similar to the behavior observed in the Go client.

func (f *sharedInformerFactory) InformerFor(obj runtime.Object, newFunc internalinterfaces.NewInformerFunc) cache.SharedIndexInformer {
	f.lock.Lock()
	defer f.lock.Unlock()

	informerType := reflect.TypeOf(obj)
	informer, exists := f.informers[informerType]
	if exists {
		return informer
	}

	resyncPeriod, exists := f.customResync[informerType]
	if !exists {
		resyncPeriod = f.defaultResync
	}

	informer = newFunc(f.client, resyncPeriod)
	informer.SetTransform(f.transform)
	f.informers[informerType] = informer

	return informer
}

No response

Client Version
e.g. 8.0.2

Kubernetes Version
e.g. 1.19.3

Java Version
e.g. Java11

[yue9944882]

thank you for capturing this and i agree it's a good improvement to make. a PR would be appreciated.

[Sud0x67]

Hi, yue9944882. Thanks for your response. I'm leaning towards making the SharedInformerFactory stay consistent with the go-client. I'll submit a PR later.

[Sud0x67]

Hi @yue9944882,

Upon closely reading the code, it seems illogical to simply return the existing informer by apiType, as developers may run into issues with being unable to modify informers once they're created.

To address this, I propose implementing a basic factory that creates informers without caching and developing a cached factory, or "informer manager," which would manage informers according to certain rules, such as cached informers by apiType.

I'd love to hear your thoughts on this proposal.

[Sud0x67]

Hello @yue9944882,

I have developed a straightforward implementation, available here: GitHub Repository. In this update, I introduced a SharedInformerManager, which organizes informers by apiClassType, along with a SimpleSharedInformerFactory dedicated to creating informers only. The existing behaviors of SharedInformerManager remain unchanged and marked as deprecated.

I plan to submit a pull request after adding more unit tests. Could you please provide some early-stage feedback on this approach? Your insights would be greatly appreciated.

[yue9944882]

instead of introducing a new SharedInformerManager interface, IMHO i think it's easier to add a new constructor parameter to the existing SharedInformerFactory like a simple boolean reuseExistingCachedInformer? i think it might be overkill to refactor the class throughout.

[Sud0x67]

Hey @yue9944882, I've submitted a PR as you suggested. Could you please review it? thx.

Here's the link: #3856