-
Notifications
You must be signed in to change notification settings - Fork 86
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
Calculate topology based on registered data nodes #324
Conversation
Codecov Report
@@ Coverage Diff @@
## main #324 +/- ##
==========================================
- Coverage 40.57% 39.99% -0.59%
==========================================
Files 110 113 +3
Lines 11775 12021 +246
==========================================
+ Hits 4778 4808 +30
- Misses 6528 6741 +213
- Partials 469 472 +3
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
09f433b
to
7852938
Compare
Generally be ready to be reviewed. Test cases still need to need polished. |
}, flags.EventuallyTimeout).Should(HaveLen(1)) | ||
} | ||
|
||
// TODO: refactor the following startup logic |
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.
Any idea how we can better organize such (fine-grained) services setup @hanahmily
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.
Would you be able to use a mock to test the behavior of the selector?
I have introduced a cluster integration in #333 . We can use it to test the cluster selector.
}, flags.EventuallyTimeout).Should(HaveLen(1)) | ||
} | ||
|
||
// TODO: refactor the following startup logic |
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.
Would you be able to use a mock to test the behavior of the selector?
I have introduced a cluster integration in #333 . We can use it to test the cluster selector.
} | ||
|
||
// NewClusterNodeRegistry creates a cluster-aware node registry. | ||
func NewClusterNodeRegistry(metaRepo metadata.Repo, selector node.Selector) NodeRegistry { |
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.
#333 will use queue.Client
to listen to node events, reducing the need for the liaison node to set up two etcd watchers. Please merge this once PR is merged.
I would raise another PR since there are too many conflicts. |
CHANGES
log.