Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ a client builder for manifestworks #273

Conversation

skeeey
Copy link
Member

@skeeey skeeey commented Sep 1, 2023

Summary

a client builder for manifestworks

Related issue(s)

Fixes #

@skeeey
Copy link
Member Author

skeeey commented Sep 1, 2023

/assign @qiujian16

factory := workinformers.NewSharedInformerFactoryWithOptions(workClientSet, b.informerResyncTime, b.informerOptions...)
informers := factory.Work().V1().ManifestWorks()
manifestWorkLister := informers.Lister()
namespacedLister := manifestWorkLister.ManifestWorks(b.clusterName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried that we have multiple thread write cache.Store at the same time. Is cache.Store thread safe? What happens if a thread is updating the spec and another is updating the status or finallizer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the informer cache is a thread safe store, when we update the spec/status/finallizer, all of these actions will be handled by the reflector , the reflector will call its store to update them, so I think it should be thread safe call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some note here that client and informer is sharing the store in this implementation. We may need to revisit the implementation in the future. Ideally, the store should be only written from the server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added a todo here

Signed-off-by: Wei Liu <liuweixa@redhat.com>
@qiujian16
Copy link
Member

/approve

leave to @morvencao to tag

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, skeeey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 5, 2023
Copy link
Member

@morvencao morvencao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 5, 2023
@openshift-merge-robot openshift-merge-robot merged commit cf1ead4 into open-cluster-management-io:main Sep 5, 2023
10 checks passed
@skeeey skeeey deleted the cloudevents-builder branch September 5, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants