-
Notifications
You must be signed in to change notification settings - Fork 403
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
Iox #415 add event parameter to find service #1058
Iox #415 add event parameter to find service #1058
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1058 +/- ##
==========================================
- Coverage 76.23% 76.20% -0.04%
==========================================
Files 343 343
Lines 12953 12971 +18
Branches 1863 1868 +5
==========================================
+ Hits 9875 9884 +9
- Misses 2463 2473 +10
+ Partials 615 614 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
45375af
to
ad69dcf
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.
Could you please create an issue (or add to some existing issue) that we have to refactor the test_service_discovery.cpp
. This is pretty hard to understand and not our standard - but also not part of this PR.
{ | ||
cxx::vector<uint64_t, MAX_SERVICE_DESCRIPTIONS> intersection; | ||
|
||
ServiceDescriptionVector_t serviceInstanceResult; | ||
// Find (K1, K2) |
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.
Now that event is added could we please remove those runtime big O notation comments.
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.
I would appreciate if we could update the big O notation comments instead of removing them. It's helpful for me if you look at the code after some time has passed.
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.
I didn't check the O notation for correctness, but the branches where it's noted didn't change. Therefore, no updates needed now. Should be updated (or removed) when the prefix tree is integrated.
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.
I will update this/remove as needed.
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 in general!
{ | ||
cxx::vector<uint64_t, MAX_SERVICE_DESCRIPTIONS> intersection; | ||
|
||
ServiceDescriptionVector_t serviceInstanceResult; | ||
// Find (K1, K2) |
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.
I would appreciate if we could update the big O notation comments instead of removing them. It's helpful for me if you look at the code after some time has passed.
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
ad69dcf
to
973d973
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.
Klugscheißerman approves
Pre-Review Checklist for the PR Author
iox-#123-this-is-a-branch
)iox-#123 commit text
)git commit -s
)task-list-completed
)Notes for Reviewer
This PR adds an event parameter to the find service methods. The event search inside
ServiceRegistry
is probably not implemented in the most efficient way, but will be replaced soon when the prefix tree is integrated.Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References