Iox #415 Make `ServiceRegistry` aware of complete `ServiceDescription` #860

mossmaurice · 2021-06-21T12:06:09Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This pull request implements a new ServiceRegistry, which stores the complete ServiceDescription and takes n:m communication into account.

Will be done in follow-up PR:

Replace std::multimap with cxx::trie (will be done in Implement a prefix tree for fast string search #859)
Increase the number of services a user can search for

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
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

Closes partly service registry as a built-in topic #415

codecov · 2021-06-21T12:27:58Z

Codecov Report

Merging #860 (78d5952) into master (2685910) will increase coverage by 0.18%.
The diff coverage is 89.21%.

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
+ Coverage   75.62%   75.81%   +0.18%     
==========================================
  Files         335      335              
  Lines       12219    12269      +50     
  Branches     2037     2052      +15     
==========================================
+ Hits         9241     9302      +61     
+ Misses       2194     2176      -18     
- Partials      784      791       +7

Flag	Coverage Δ
unittests	`74.75% <87.25%> (+0.22%)`	⬆️
unittests_timing	`29.70% <35.29%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
...clude/iceoryx_posh/internal/roudi/port_manager.hpp	`100.00% <ø> (ø)`
...posh/include/iceoryx_posh/runtime/posh_runtime.hpp	`100.00% <ø> (ø)`
iceoryx_posh/source/capro/service_description.cpp	`94.68% <ø> (-0.12%)`	⬇️
iceoryx_posh/source/runtime/ipc_interface_base.cpp	`59.30% <0.00%> (ø)`
iceoryx_posh/source/roudi/process_manager.cpp	`59.94% <66.66%> (-0.42%)`	⬇️
iceoryx_posh/source/roudi/port_manager.cpp	`78.23% <78.26%> (+0.56%)`	⬆️
iceoryx_posh/source/runtime/posh_runtime_impl.cpp	`66.13% <88.88%> (+1.26%)`	⬆️
iceoryx_posh/source/roudi/service_registry.cpp	`96.92% <96.92%> (-3.08%)`	⬇️
iceoryx_hoofs/source/concurrent/loffli.cpp	`79.41% <0.00%> (-11.77%)`	⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp	`60.60% <0.00%> (-0.87%)`	⬇️
... and 4 more

elfenpiff

First part reviewed - except the tests

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

elfenpiff · 2021-06-21T14:36:02Z

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

+    /// @brief Removes given service description from registry
+    /// @param[in] serviceDescription, service to be removed
+    /// @return true, if service description was removed, false otherwise
+    bool remove(const capro::ServiceDescription& serviceDescription);


I am confused here. If we remove a service the service is definitely removed after this call! And when the services was not added it is still removed.
Therefore, independent of the preconditions the user will always get the same result - a removed service. So why do we have to return a bool here?

This method will hardly be used to check if a service is added - for this we can use find.

I think we have to be cautious some time since such return values can increase the error handling overhead which brings in some cases no benefit and only drawbacks.

I think it is intended to indicate whether the service was there and removed or was not there and not removed. The logical state of the registry is the same in both cases, so we do not really need to communicate this. Personally I do those kind of things as well for more information if the user wants (the information was already computed and passing it is inexpensive). The user is technically free to ignore the return value (I know there are differing opinions on this one).

Regarding error handling overhead: the return value does not indicate a possible error, just further information on what actually happened.

This kind of style is definitely used in several APIs, but I am fine either way.

I think it is intended to indicate whether the service was there and removed or was not there and not removed.

Exactly, that's the idea. There shall be the possibility to check whether remove was successful or remove was unsuccessful, when the element was not contained in the registry.

And when the services was not added it is still removed.

That's not entirely correct. It will not be removed if it wasn't added before. The bool is used to signal this. I'll keep the additional info to be able to pass this to the user via the new ServiceDiscovery API.

@mossmaurice I disagree. The reason is, what can a user do with this kind of information: One removes a service and after the call it is not there - this is the contract. If you return false I would assume that you tried to remove the service and failed therefore it is still present.

In my opinion it does not make sense to return here a bool. Do you have an argument for that? And if so, why not return an enum so that it is more readable and one really knows what is going on!
Please also consider that we should try to reduce unnecessary code bloat and error handling. If we do return here something we are forced by our standards to handle it and we turn this line

serviceRegistry.remove(myService);

into

if ( serviceRegistry.remove(myService) ) { //... } else { //... }

And as final argument: YAGNI, you aint gonna need it. I do not see the use case and if the use case arises we can still add it.

iceoryx_posh/source/roudi/service_registry.cpp

elfenpiff · 2021-06-21T15:07:06Z

iceoryx_posh/source/roudi/service_registry.cpp

+    {
+        if (element == serviceDescription)
+        {
+            return cxx::error<ServiceRegistryError>(ServiceRegistryError::SERVICE_DESCRIPTION_ALREADY_ADDED);


This error just increases the error handling overhead and has no value for the user. If a user calls add, there are two possibilities:

it is not added, but after the call -> it is added

it is added, and after the call -> it is still added

Therefore, the user does not require any action here and if they would like to know if a certain service was registered they always can use find.

Yes, it is also not an error if it already exists. Whether the user should be informed is debatable. I am fine with either way, just do not classify it as error.

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

elfenpiff · 2021-06-21T15:13:43Z

iceoryx_posh/source/roudi/service_registry.cpp

 {
-    cxx::set::remove(m_serviceMap[service].instanceSet, instance);


Please use the suggested data structure: std::map<std::map<CaproIdString_t, std::set<CaproIdString_t>>, CaproIdString_t> m_services and refactor this method.

The implementation would be.

Remove event entry from std::set<CaproIdString_t>

If the set from 1. is empty remove the instance entry from std::map<CaproIdString_t, std::set<CaproIdString_t>>

If the map from 2. is empty remove the service entry from `m_services.

You can safely remove m_serviceDescriptionVector, m_instanceMap and m_serviceMap since the data structure from above is sufficient enough

As described in another comment this structure is inefficient for storage (considering we would need static capacities for all of them) and also and does not lend itself for efficient operations, especially wildcard search. Furthermore the way it is proposed it it does not allow us to locate a full ServiceDescription object which will contain more than 3 strings in the foreseeable future - we will need to embed custom information of DDS topics I think (like QoS), have not fully thought it through. But even if we do not need it for QoS, it is good to have to attach all information about a service here in one place, or at least point to it.

I am also not sure how a map as key would really behave, since it has no natural operator< which is a requirement for key types (would maybe fall back to addresses of the keys here, but this could be problematic). Besides any map where keys or values are more than lightweight objects is doomed efficiency wise, since insertion and removal require copies of internal data (which would consist of maps of sets here). This is just not going to work efficiently

See my answer in the other thread

To elaborate: Map keys need operator<, but if it does not exist it will use std::less in the map I think and use comparison on pointers to dynamically allocated data objects (allocated by the map). In other words: pointers to memory are the real key in this case, which is fine. Not fully sure though, our own map would have to behave in some reasonable way there as well and I do not know yet what this would be.

The storage size problem would be severe in any case for nested fixed-size containers.
This means we would need to implement a complex solution with a lot of drawbacks. We can have a simpler (still fairly complex) solution which at least avoids some of the drawbacks, is faster, generalizes in a better way to e.g. more keys, allows prefix based search and more.
The data structure itself is also somewhat easier to implement. But I fully intend to create the fixed-size map as well, just that we can design better algorithms use suitable abstractions instead for other problems.

elfenpiff · 2021-06-21T15:29:54Z

iceoryx_posh/source/roudi/service_registry.cpp

                           const CaproIdString_t& service,
                           const CaproIdString_t& instance) const
 {
-    if (instance == iox::cxx::string<100>(capro::AnyInstanceString))


Please refactor find as well with the suggested data structure: std::map<std::map<CaproIdString_t, std::set<CaproIdString_t>>, CaproIdString_t> m_services.

Then you can write the algorithm with much less code, somehow like this:

cxx::vector<uint64_t, MAX_SERVICE_DESCRIPTIONS> searchResult; auto serviceStartIter = (service == capro::AnyServiceString) ? m_service.begin() : m_services.find(service); auto serviceEndIter = (service == capro::AnyServiceString) ? m_service.end() : serviceStartIter + 1; for(auto iter = serviceStartIter; iter != m_service.end() && iter != serviceEndIter; ++iter) { auto instanceStartIter = (service == capro::AnyInstanceString) ? iter.begin() : iter.find(service); auto instanceEndIter = (service == capro::AnyInstanceString) ? iter.end() : instanceStartIter + 1; for(auto iter2 = instanceStartIter; iter2 != iter.end() && iter2 != instanceEndIter; ++iter2) { for(auto event : iter2 ) searchResult.emplace_back(iter.first, iter2.first, event); } } // maybe you have to rename iter to serviceIter, iter2 to instanceIter but this could would be much cleaner and faster

I need to look a at the current implementation but I am sure that any solution with maps and sets is inferior to prefix tree implementations I propose (and the latter is easier to implement than a map or set, especially with our memory constraints).

The reason is simple: completing most operations in O(length of string to insert), as opposed to anything with maps which at least need O(log(#elements in container)*length of keys to compare) - and this is for find only, not considering for loops or anything else. These structures are much harder to implement with the logarithmic guarantees as well.

I have not checked the correctness of the above code, but if it is correct the estimate still holds true and gets worse with any nested loop iteration with find, as above.

Also we should not mix up short concise code with runtime-efficient code. For cases like this it is at least worth considering order of complexity of the algorithm (as above, I just gave lower bounds for comparison but the prefix tree order is tight). There are a lot of nested iterations going on, this is a bad sign. Of course we can argue that the results are small in most cases, but not in the worst and especially the wildcard case.

Nevertheless we can have concise and efficient code here at the same time with a combination of prefix trees for search for specific attributes (strings) and the right container abstraction for the elements themselves, which would be something like a minimal wrapper around a vector (actually array) or the ObjectPool.

@elfenpiff That's a neat and small implementation of find()! However, as stated in the other threads I'm not convinced that we will be able to build a fixed size cxx::map, that can be place in shared memory. What is your take on that?

This should be feasible. I see a map implementation as a challenge but not the static memory part and in some previous company I already have seen such a map. The tree can be implemented in the same fashion like the linked list which we already have (from the memory point of view).

This is true, it is feasible with the same approach as the list, but it would still be the wrong structure for the job due to this nesting of maps and sets.

And it is a challenge I intend to tackle in the near future when we have a working approach. We would even be able to compare efficiency then, and my bet is against any map based approach (we can try with std::map, which is likely better optimized then what we will be able to come up in cxx::map without serious time investment).

elBoberido

service registry tests not yet reviewed

iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp

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

iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp

iceoryx_posh/source/roudi/port_manager.cpp

iceoryx_posh/source/roudi/service_registry.cpp

elBoberido · 2021-06-28T14:54:16Z

iceoryx_posh/source/roudi/service_registry.cpp

+                if (it->second == index)
+                {
+                    removedValue = it->second;
+                    it = m_serviceMap.erase(it);
+                    removedEntry = true;
+                    continue;
+                }
+                else if (removedEntry && it->second > removedValue)
+                {
+                    // update index due to removed element
+                    it->second--;
+                }


This is broken. What happens when there is an entry with it->second > removedIndex before the key was removed from the map?
removedValue and removedEntry are also not necessary. Just use index and maybe rename it to removedIndex, then the previous mentioned defect will also be fixed

The latter branch is only taken if a matching index was found aka removedEntry == true. However, you're right that things are a bit entangled here. Refactored to make it more understandable.

In your new fix PR I see that the iterator is always incremented but not part of the for loop header, could you please change this: #917

iceoryx_posh/test/integrationtests/test_roudi_findservice.cpp

iceoryx_posh/source/roudi/service_registry.cpp

elBoberido · 2021-06-28T15:27:27Z

iceoryx_posh/source/roudi/service_registry.cpp

+            auto range = m_serviceMap.equal_range(service);
+            for (auto entry = range.first; entry != range.second; ++entry)
+            {
+                searchResult.push_back(m_serviceDescriptionVector[entry->second]);
+            }


Since this is used quite often, can we have a lambda which could be used like

storeIndices(containerToStore, range);

With only two occurrences, I prefer to keep things how they are. ServiceRegistry::find is very readable and fits well on one monitor currently.

well, I count 4 ;)

any input on this? Resolving would be fine since the duplicated code is minimal, but you might revise your decision with four occurrences ;)

any input on this?

Still only count two occurrences 🤔 Let's leave it as it is 😇

@elBoberido can you list your for occurrences here please so that @mossmaurice can see them as well.

… database efficently (WIP) Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

… and write down new test cases Signed-off-by: Simon Hoinkis <[email protected]>

…dServiceSingleInstance Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

…rviceRegistry::Error Signed-off-by: Simon Hoinkis <[email protected]>

…olete forward declaration Signed-off-by: Simon Hoinkis <[email protected]>

…Registry fails Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

…stry Signed-off-by: Simon Hoinkis <[email protected]>

…emove} Signed-off-by: Simon Hoinkis <[email protected]>

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice · 2021-08-04T14:01:01Z

However, erasing elements from m_serviceDescriptionVector will invalidate indices currently in the maps and cause subsequent lookups and removals to behave incorrectly.

Please have a look at the test case MultipleServiceDescriptionAddedInNonLinearOrderFindsCorrectServices. With this test case the correct removal behaviour is verified. After the element is removed from the vector, the index is updated in the two maps. I agree that is not the most efficient way and I'm happy to refactor it once the ObjectPool is cleaned up and ready to use in a follow-up PR.

…removing a ServiceDescription from the ServiceRegistry and add related test cases Signed-off-by: Simon Hoinkis <[email protected]>

…#415-make-service-registry-aware-of-complete-service-description

CHANGELOG.md

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

iceoryx_posh/include/iceoryx_posh/runtime/posh_runtime.hpp

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

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

iceoryx_posh/source/roudi/service_registry.cpp

elfenpiff · 2021-08-06T09:10:58Z

iceoryx_posh/source/roudi/service_registry.cpp

+    cxx::vector<uint64_t, MAX_SERVICE_DESCRIPTIONS> intersection;
+
+    // Find (K1, K2)
+    // O(log n + log n + max(#PossibleServices + #possiblesInstances) + #intersection)


You can write it much simpler, it is just: O(log(n))

Here is why:

log n + log n = log ( n * n ) = log ( n ^ 2 ) log ( n ^ 2 ) = 2 * log n

See: https://en.wikipedia.org/wiki/List_of_logarithmic_identities

And the big-O-notation: https://en.wikipedia.org/wiki/Big_O_notation states
that O(log(n)) that the Find(K1, K2) function has an upper limit which is log(n) with some constant.
Find(K1, K2) = O(log(n)) => Find(K1, K2) <= M * log(n) with M > 0. So the M constant contains the factor 2 and all the constants with max(#PossibleServices + #possibleInstances) + #intersection).

@elfenpiff The first part is true, but we do not need the logarithm to argue this since it holds for any function f and constant C that O(C*f(n)) = O(f(n)) simply by definition.

The second part is wrong, the result set still may dominate this so it is O(log n + #resultset) if we want to be exact. #resultset is not bounded by a constant and may depend on n. In the worst case it is n itself and naively we would then have O(n) (no algorithm can be better in this case). To get a better estimate we introduce #resultset as a second variable and essentially use multivariate O-notation.

In general #resultset will be small so we can think of this as O(log n) for practical purposes but strictly speaking it is not.

We hence can simplify this to O(log n + #resultset), but not O(log n). It maybe helps to see #resultset as some function of n which depends on the input query as well but is at most linear. We do not know more for general input and cannot improve the estimate for this reason.

Note that #resultset is the size of the intersection above. Computing the intersection takes linear time in the size of the operand sets if they are sorted. Note that the #resultset estimate I gave is also slightly wrong for this reason since computing the resultset can be more costly (say the result contains only 1 element but one of the operands has say 0.7*n candidates, computation will be linear in n in this case). If we keep the max as before this is accounted for, just remove one of the log n terms.

TLDR: the estimate above with one log n term removed is as correct and cannot really be simplified without cheating.

iceoryx_posh/source/roudi/service_registry.cpp

CHANGELOG.md

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

iceoryx_posh/source/roudi/service_registry.cpp

iceoryx_posh/source/capro/service_description.cpp

Signed-off-by: Simon Hoinkis <[email protected]>

elBoberido

Sorry, tests still not reviewed.

There are also quite a few issues which are deferred to a follow up PR. Please document these like the the other todo #issueNumber else it is quite hard to keep track of this

iceoryx_posh/source/runtime/posh_runtime_impl.cpp

dkroenke · 2021-08-27T16:51:51Z

iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp

@@ -43,23 +43,175 @@ class ServiceRegistry_test : public Test
        }
    }
    iox::roudi::ServiceRegistry registry;
-    iox::roudi::ServiceRegistry::InstanceSet_t searchResults;
+
+    iox::roudi::ServiceRegistry::ServiceDescriptionVector_t searchResults;


should be named with sut

Ok, will do this with the next PR ;)

dkroenke · 2021-08-27T17:10:23Z

iceoryx_posh/test/moduletests/test_roudi_service_registry.cpp


    registry.find(searchResults, "a", "c");
    EXPECT_THAT(searchResults.size(), Eq(0));
 }

-TEST_F(ServiceRegistry_test, RemoveSingleFromMultipleServices)
+TEST_F(ServiceRegistry_test,
+       AddingMultipleServiceDescriptionWithDifferentServicesAndRemovingSpecificDoesNotFindSpecific)


Can surely be reworded 😄

Suggested change

AddingMultipleServiceDescriptionWithDifferentServicesAndRemovingSpecificDoesNotFindSpecific)

ServiceNotFoundAfterAddingAndRemovingToServiceRegistry)

Will reword it in the next PR.

dkroenke · 2021-08-27T17:36:09Z

Sorry, tests still not reviewed.

There are also quite a few issues which are deferred to a follow up PR. Please document these like the the other todo #issueNumber else it is quite hard to keep track of this

@elBoberido I've reviewed the tests.

budrus · 2021-08-27T18:59:47Z

CHANGELOG.md

+// after
+discoveryInfo.offerService(myServiceDescription);
+discoveryInfo.stopOfferService(myServiceDescription);
+discoveryInfo.findService("ServiceA", Wildcard);


Does this mean that we now call findService, provide a service ID and what will be returned is a container with full CaProServiceDescription? The Service IDs in this Service Description would be some kind of redundant information as hopefully you only get CaPro IDs that match the requested service ID

That's correct. Service IDs in this case would all be "ServiceA". IMHO this is beneficial to the old design as the user will now be able to read all the other members of a ServiceDescription. This could in future be also e.g. QoS settings.

To be precise: The return value is a vector wrapped in an expected cxx::expected<ServiceContainer, FindServiceError> and using ServiceContainer = iox::cxx::vector<capro::ServiceDescription, MAX_NUMBER_OF_SERVICES>;

renaming test cases and member variable Signed-off-by: Simon Hoinkis <[email protected]>

… fixing ServiceDescription deserialisation Signed-off-by: Simon Hoinkis <[email protected]>

renaming test cases and member variable Signed-off-by: Simon Hoinkis <[email protected]>

… fixing ServiceDescription deserialisation Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added the refactoring Refactor code without adding features label Jun 21, 2021

mossmaurice requested review from elBoberido and MatthiasKillat June 21, 2021 12:06

mossmaurice self-assigned this Jun 21, 2021

elfenpiff reviewed Jun 21, 2021

View reviewed changes

mossmaurice removed the request for review from elBoberido June 21, 2021 15:41

elBoberido requested review from elBoberido and removed request for elBoberido June 21, 2021 16:52

elBoberido reviewed Jun 28, 2021

View reviewed changes

mossmaurice mentioned this pull request Jun 29, 2021

Iox #415 Refactor Service Description Part3: Restrict wildcard usage to searching only #854

Merged

20 tasks

mossmaurice added 14 commits June 30, 2021 17:34

iox-eclipse-iceoryx#415 Draft two attempts to access service registry…

a51eeff

… database efficently (WIP) Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Implement first version of new ServiceRegistry

88fea8f

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Remove obsolete comments

02071d4

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Send real ServiceDescription over IPC channel…

973b166

… and write down new test cases Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix RoudiFindService_test.StopOfferMultiMetho…

e20c118

…dServiceSingleInstance Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix RoudiFindService_test.InterfacePort

5105289

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add asserts to RoudiFindService_test

ea0ed42

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add copyright header and doxygen

fb20956

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Update some comments

b7c3f97

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Write down some more ServiceRegistry test cases

5e0f5b0

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add noexcept to ServiceRegistry and rename Se…

8e24029

…rviceRegistry::Error Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add todo for new discovery API and remove obs…

486a008

…olete forward declaration Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add new error in case adding entry to Service…

d2c0aaf

…Registry fails Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix PoshRuntime and FindService tests

cef63b5

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#415-make-service-registry-aware-of-complete-service-description branch from 94ef01d to cef63b5 Compare June 30, 2021 15:48

mossmaurice added 2 commits July 1, 2021 11:12

iox-eclipse-iceoryx#415 Implement out-commented tests for ServiceRegi…

2fec024

…stry Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Add validity check to ServiceRegistry::{add,r…

479ea0b

…emove} Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Remove Error::SERVICE_DESCRIPTION_ALREADY_ADDED

27271df

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#415-make-service-registry-aware-of-complete-service-description branch from cd37e47 to 27271df Compare August 3, 2021 13:43

iox-eclipse-iceoryx#415 Create temp variable serviceIndex

43a4f7f

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Take the reference counter into account when …

f862b44

…removing a ServiceDescription from the ServiceRegistry and add related test cases Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice requested review from elBoberido and MatthiasKillat August 4, 2021 14:33

Merge remote-tracking branch 'origin/master' into iox-eclipse-iceoryx…

58dfb50

…#415-make-service-registry-aware-of-complete-service-description

elfenpiff reviewed Aug 6, 2021

View reviewed changes

MatthiasKillat reviewed Aug 9, 2021

View reviewed changes

mossmaurice added 4 commits August 9, 2021 10:54

iox-eclipse-iceoryx#415 Use ServiceDescriptionEntry instead of cxx::pair

f8b3ff6

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix some typos

41b8294

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix U's and ServiceRegistry::remove

bb8a348

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#415 Fix code formatting

78d5952

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice requested a review from MatthiasKillat August 9, 2021 11:31

MatthiasKillat approved these changes Aug 9, 2021

View reviewed changes

elBoberido reviewed Aug 21, 2021

View reviewed changes

iceoryx_posh/source/runtime/posh_runtime_impl.cpp Show resolved Hide resolved

mossmaurice mentioned this pull request Aug 27, 2021

service registry as a built-in topic #415

Closed

25 tasks

dkroenke reviewed Aug 27, 2021

View reviewed changes

dkroenke self-requested a review August 27, 2021 17:34

dkroenke approved these changes Aug 27, 2021

View reviewed changes

budrus reviewed Aug 27, 2021

View reviewed changes

mossmaurice merged commit 816270e into eclipse-iceoryx:master Aug 30, 2021

mossmaurice added a commit to ApexAI/iceoryx that referenced this pull request Sep 1, 2021

iox-eclipse-iceoryx#415 Address reviewing finds from eclipse-iceoryx#860

19bf1eb

renaming test cases and member variable Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added a commit to ApexAI/iceoryx that referenced this pull request Sep 1, 2021

iox-eclipse-iceoryx#415 Address review finding from eclipse-iceoryx#860…

a8e6082

… fixing ServiceDescription deserialisation Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added a commit to ApexAI/iceoryx that referenced this pull request Sep 1, 2021

iox-eclipse-iceoryx#415 Address review findings from eclipse-iceoryx#860

0397cec

renaming test cases and member variable Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added a commit to ApexAI/iceoryx that referenced this pull request Sep 1, 2021

iox-eclipse-iceoryx#415 Address review finding from eclipse-iceoryx#860…

0af7817

… fixing ServiceDescription deserialisation Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice mentioned this pull request Sep 1, 2021

iox-#415 Rename variable and test case name in ServiceRegistry_test and fix bug in ServiceDescription c'tor #917

Merged

20 tasks

		{
		cxx::set::remove(m_serviceMap[service].instanceSet, instance);

	AddingMultipleServiceDescriptionWithDifferentServicesAndRemovingSpecificDoesNotFindSpecific)
	ServiceNotFoundAfterAddingAndRemovingToServiceRegistry)

Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860

Iox #415 Make ServiceRegistry aware of complete ServiceDescription #860

Conversation

mossmaurice commented Jun 21, 2021 • 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 Jun 21, 2021 • edited Loading

Codecov Report

elfenpiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Jun 21, 2021 • 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

MatthiasKillat Jun 21, 2021 • 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

MatthiasKillat Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elfenpiff Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

elBoberido left a 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

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

mossmaurice commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

elBoberido left a 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

dkroenke commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iox #415 Make `ServiceRegistry` aware of complete `ServiceDescription` #860

Iox #415 Make `ServiceRegistry` aware of complete `ServiceDescription` #860

mossmaurice commented Jun 21, 2021 •

edited

Loading

codecov bot commented Jun 21, 2021 •

edited

Loading

MatthiasKillat Jun 21, 2021 •

edited

Loading

MatthiasKillat Jun 21, 2021 •

edited

Loading

MatthiasKillat Jun 21, 2021 •

edited

Loading

elfenpiff Jun 22, 2021 •

edited

Loading

MatthiasKillat Jun 22, 2021 •

edited

Loading

MatthiasKillat Aug 9, 2021 •

edited

Loading