-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ENH] Adds memberlist manager #1354
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
170f939
to
e5e4ce0
Compare
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major comments. Am I correct in my understanding that this is just an initial scaffolding for the controller, which will later take care of managing workers?
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
|
||
func (s *CRMemberlistStore) GetMemberlist() (*Memberlist, error) { | ||
gvr := getGvr() | ||
unstrucuted, err := s.dynamicClient.Resource(gvr).Namespace("chroma").Get(context.TODO(), "worker-memberlist", metav1.GetOptions{}) //.Namespace(m.coordinator_namespace).Get(context.TODO(), m.memberlist_custom_resource, metav1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: investigate how to use context properly (pass into struct?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go/coordinator/internal/memberlist_manager/memberlist_manager_test.go
Outdated
Show resolved
Hide resolved
e5e4ce0
to
174ef57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending addressing review comments.
We need to followup on:
- Batch event processing to smooth start up and transient failures.
- Evaluate whether we need to customized CRD clients. We may need more CRDs to handle topic reconfiguration, ddl and TSOs.
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
go/coordinator/internal/memberlist_manager/memberlist_manager.go
Outdated
Show resolved
Hide resolved
9660a1e
to
ed94908
Compare
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
New unit tests were added for this case for each module
pytest
for python,yarn test
for jsDocumentation Changes
None