Iox #415 remove InvalidIdString and isValid() from ServiceDescription #1047

FerdinandSpitzschnueffler · 2022-01-26T11:58:53Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR removes InvalidIdString and isValid() from ServiceDescription and replaces the Wildcard string with a nullopt. With this, the ServiceDescription can contain any string, empty strings by default.

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
- Each unit test case has a unique UUID
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Partially closes service registry as a built-in topic #415

codecov · 2022-01-26T12:24:44Z

Codecov Report

Merging #1047 (2d372eb) into master (da830c1) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1047      +/-   ##
==========================================
- Coverage   77.55%   77.54%   -0.01%     
==========================================
  Files         340      340              
  Lines       12827    12815      -12     
  Branches     1847     1838       -9     
==========================================
- Hits         9948     9938      -10     
- Misses       2260     2263       +3     
+ Partials      619      614       -5

Flag	Coverage Δ
unittests	`76.76% <50.00%> (+<0.01%)`	⬆️
unittests_timing	`12.71% <6.81%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...eoryx_hoofs/include/iceoryx_hoofs/cxx/helplets.hpp	`88.37% <ø> (ø)`
...include/iceoryx_posh/capro/service_description.hpp	`100.00% <ø> (ø)`
...clude/iceoryx_posh/internal/roudi/port_manager.hpp	`100.00% <ø> (ø)`
...de/iceoryx_posh/internal/roudi/process_manager.hpp	`100.00% <ø> (ø)`
...oryx_posh/include/iceoryx_posh/roudi/port_pool.hpp	`100.00% <ø> (ø)`
...posh/include/iceoryx_posh/runtime/posh_runtime.hpp	`100.00% <ø> (ø)`
...include/iceoryx_posh/runtime/service_discovery.hpp	`0.00% <ø> (ø)`
iceoryx_posh/source/roudi/port_manager.cpp	`78.38% <0.00%> (+1.19%)`	⬆️
iceoryx_posh/source/roudi/process_manager.cpp	`57.26% <ø> (ø)`
iceoryx_posh/source/roudi/roudi.cpp	`57.71% <0.00%> (-0.59%)`	⬇️
... and 9 more

iceoryx_posh/include/iceoryx_posh/internal/roudi/port_manager.hpp

iceoryx_posh/include/iceoryx_posh/internal/roudi/roudi.hpp

iceoryx_hoofs/include/iceoryx_hoofs/cxx/helplets.hpp

iceoryx_posh/include/iceoryx_posh/runtime/service_discovery.hpp

iceoryx_posh/source/capro/service_description.cpp

iceoryx_posh/source/popo/ports/interface_port_data.cpp

mossmaurice · 2022-01-27T09:48:55Z

iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp

    /// @param[in] searchResult, reference to the vector which will be filled with the results
    /// @param[in] service, string or wildcard to search for
    /// @param[in] instance, string or wildcard to search for
    void find(ServiceDescriptionVector_t& searchResult,
-              const capro::IdString_t& service = Wildcard,
-              const capro::IdString_t& instance = Wildcard) const noexcept;
+              const cxx::variant<cxx::Wildcard_t, capro::IdString_t>& service,


You could think about keeping the default argument Wildcard

Not sure this is possible with variant without wrapping it, i.e. constructing a variant with Wildcard. In any case I prefer overloads in theory, i.e. have a call with service, one with instance, one with both and one without arguments. This improves dispatching as well and internally the cases are implemented in a switch statement otherwise anyway.

The reasoning is to prefer early dispatching mechanisms if possible.

@mossmaurice I decided against because it was not used anywhere, not even tested, and as @MatthiasKillat wrote, it's not that easy with a variant.

mossmaurice · 2022-01-27T09:51:28Z

iceoryx_posh/include/iceoryx_posh/runtime/service_discovery.hpp

@@ -41,8 +42,8 @@ class ServiceDiscovery
    /// ServiceContainer: on success, container that is filled with all matching instances
    /// FindServiceError: if any, encountered during the operation
    cxx::expected<ServiceContainer, FindServiceError>
-    findService(const cxx::variant<Wildcard_t, capro::IdString_t> service,
-                const cxx::variant<Wildcard_t, capro::IdString_t> instance) noexcept;
+    findService(const cxx::variant<cxx::Wildcard_t, capro::IdString_t>& service,


Because that's part of the user API it should be IMHO in runtime.

Suggested change

findService(const cxx::variant<cxx::Wildcard_t, capro::IdString_t>& service,

findService(const cxx::variant<runtime::Wildcard_t, capro::IdString_t>& service,

I think the Wildcard belongs to the same namespace as the ServiceDescription but we may change some of those later, so I am not sure yet and until we know it is not important.

I agree with @MatthiasKillat and moved the Wildcard (now a nullopt) to the ServiceDescription, so the namespace is now capro.

mossmaurice · 2022-01-27T09:53:04Z

iceoryx_posh/source/roudi/port_manager.cpp

-    // send find to all interfaces
-    capro::CaproMessage caproMessage(capro::CaproMessageType::FIND, {service, instance, roudi::Wildcard});
-
-    for (auto interfacePortData : m_portPool->getInterfacePortDataList())
-    {
-        iox::popo::InterfacePort interfacePort(interfacePortData);
-        interfacePort.dispatchCaProMessage(caproMessage);
-    }
-


Why was this part removed?

I also do not understand this, but I suspect it has something to do with the FIND message being changed and interface ports not being used properly (in the sense that they will not answer the request anyway).

But that is just a guess and requires clarification. @FerdinandSpitzschnueffler

After changing the findService parameters, this code had to be adapted since the CaproMessage c'tor requires a ServiceDescription. I discussed several ideas with @elfenpiff, but then we came to the conclusion that the CaproMessage itself could need some refactoring and that we can send the find here but there will be no answer anyway as @MatthiasKillat stated.

iceoryx_posh/source/roudi/service_registry.cpp

iceoryx_posh/include/iceoryx_posh/capro/service_description.hpp

MatthiasKillat

Looks OK, minor clarifications needed and I consider the variant interface problematic in the long-term but this PR is not the place to change that.

iceoryx_posh/source/capro/service_description.cpp

iceoryx_posh/source/roudi/roudi.cpp

iceoryx_posh/source/roudi/service_registry.cpp

MatthiasKillat · 2022-01-27T11:18:42Z

iceoryx_posh/source/runtime/service_discovery.cpp

 {
    /// @todo #415 remove the string mapping, once the find call is done via shared memory
    capro::IdString_t serviceString;
    capro::IdString_t instanceString;
+    bool isServiceWildcard = false;
+    bool isInstanceWildcard = false;

    if (service.index() == 0U)


We could maybe also introduce a helper function

bool isWildCard(const cxx::variant< ... >& value) { return value.index() == 0U; }

This will be inlined and make the code more expressive.

MatthiasKillat · 2022-01-27T11:27:02Z

iceoryx_posh/source/roudi/port_manager.cpp

-    // send find to all interfaces
-    capro::CaproMessage caproMessage(capro::CaproMessageType::FIND, {service, instance, roudi::Wildcard});
-
-    for (auto interfacePortData : m_portPool->getInterfacePortDataList())
-    {
-        iox::popo::InterfacePort interfacePort(interfacePortData);
-        interfacePort.dispatchCaProMessage(caproMessage);
-    }
-


I also do not understand this, but I suspect it has something to do with the FIND message being changed and interface ports not being used properly (in the sense that they will not answer the request anyway).

But that is just a guess and requires clarification. @FerdinandSpitzschnueffler

CHANGELOG.md

…istry Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler added the refactoring Refactor code without adding features label Jan 26, 2022

FerdinandSpitzschnueffler self-assigned this Jan 26, 2022

dkroenke self-requested a review January 26, 2022 12:38

elfenpiff reviewed Jan 26, 2022

View reviewed changes

FerdinandSpitzschnueffler requested a review from mossmaurice January 26, 2022 16:39

mossmaurice reviewed Jan 27, 2022

View reviewed changes

MatthiasKillat reviewed Jan 27, 2022

View reviewed changes

iceoryx_posh/include/iceoryx_posh/capro/service_description.hpp Outdated Show resolved Hide resolved

MatthiasKillat reviewed Jan 27, 2022

View reviewed changes

dkroenke removed their request for review January 27, 2022 12:54

FerdinandSpitzschnueffler force-pushed the iox-#415-remove-is-valid-from-service-description branch 2 times, most recently from aae8eae to 4fe9b1d Compare January 28, 2022 13:14

FerdinandSpitzschnueffler mentioned this pull request Jan 28, 2022

service registry as a built-in topic #415

Closed

25 tasks

FerdinandSpitzschnueffler requested review from mossmaurice, elfenpiff and MatthiasKillat January 28, 2022 13:21

elfenpiff reviewed Jan 28, 2022

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

MatthiasKillat previously approved these changes Jan 28, 2022

View reviewed changes

FerdinandSpitzschnueffler added 8 commits January 31, 2022 10:27

iox-eclipse-iceoryx#415 Revise doxygen in service description and reg…

e7de55e

…istry Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Remove isValid method and InvalidIdString

fc874f8

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Remove Wildcard string

8043692

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Update CHANGELOG, test names, years

678f976

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Move Wildcard struct to ServiceDescription

b45ed20

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Replace variant with optional

286b781

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Change return type of get*IDString()

cd14744

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

iox-eclipse-iceoryx#415 Remove obsolete tests

2d372eb

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>

FerdinandSpitzschnueffler dismissed MatthiasKillat’s stale review via 2d372eb January 31, 2022 09:33

FerdinandSpitzschnueffler force-pushed the iox-#415-remove-is-valid-from-service-description branch from 4fe9b1d to 2d372eb Compare January 31, 2022 09:33

MatthiasKillat approved these changes Jan 31, 2022

View reviewed changes

elfenpiff approved these changes Jan 31, 2022

View reviewed changes

FerdinandSpitzschnueffler merged commit 58f17f2 into eclipse-iceoryx:master Jan 31, 2022

FerdinandSpitzschnueffler deleted the iox-#415-remove-is-valid-from-service-description branch January 31, 2022 10:05

FerdinandSpitzschnueffler mentioned this pull request Feb 21, 2022

Refactoring of ServiceDescription #1141

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #415 remove InvalidIdString and isValid() from ServiceDescription #1047

Iox #415 remove InvalidIdString and isValid() from ServiceDescription #1047

FerdinandSpitzschnueffler commented Jan 26, 2022 •

edited

Loading

codecov bot commented Jan 26, 2022 •

edited

Loading

mossmaurice Jan 27, 2022 •

edited

Loading

MatthiasKillat Jan 27, 2022 •

edited

Loading

FerdinandSpitzschnueffler Jan 27, 2022

mossmaurice Jan 27, 2022

MatthiasKillat Jan 27, 2022

FerdinandSpitzschnueffler Jan 28, 2022

mossmaurice Jan 27, 2022

MatthiasKillat Jan 27, 2022

FerdinandSpitzschnueffler Jan 27, 2022

MatthiasKillat left a comment

MatthiasKillat Jan 27, 2022 •

edited

Loading

MatthiasKillat Jan 27, 2022

	findService(const cxx::variant<cxx::Wildcard_t, capro::IdString_t>& service,
	findService(const cxx::variant<runtime::Wildcard_t, capro::IdString_t>& service,

Iox #415 remove InvalidIdString and isValid() from ServiceDescription #1047

Iox #415 remove InvalidIdString and isValid() from ServiceDescription #1047

Conversation

FerdinandSpitzschnueffler commented Jan 26, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Jan 26, 2022 • edited Loading

Codecov Report

mossmaurice Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat left a comment

Choose a reason for hiding this comment

MatthiasKillat Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FerdinandSpitzschnueffler commented Jan 26, 2022 •

edited

Loading

codecov bot commented Jan 26, 2022 •

edited

Loading

mossmaurice Jan 27, 2022 •

edited

Loading

MatthiasKillat Jan 27, 2022 •

edited

Loading

MatthiasKillat Jan 27, 2022 •

edited

Loading