iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] #1084

elBoberido · 2022-02-09T17:40:56Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR adds the client and server to the PortPool and PortManager. With this changes, the discovery loop is also updated to handle the new ports and it is taken care of resource cleanup.

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

Relates to Request/Response communication with iceoryx #27

elfenpiff

In my opinion we should add to every Log* message as much information as possible since we do not know what may becomes important during debugging.
Furthermore, I would suggest to always call LogError before calling the errorHandler to provide some context to the user. What do you think?

The rest looks fine - only minor things.

elfenpiff · 2022-02-11T09:23:25Z

iceoryx_binding_c/include/iceoryx_binding_c/publisher.h

@@ -40,7 +40,7 @@ typedef struct
    bool offerOnCreate;

    /// @brief describes whether a publisher blocks when subscriber queue is full


Please adjust the comment.

Why, the comment is stil true. It's only the data type that changed it's name, note the member

iceoryx_binding_c/include/iceoryx_binding_c/internal/c2cpp_enum_translation.hpp

iceoryx_binding_c/include/iceoryx_binding_c/runtime.h

elfenpiff · 2022-02-11T09:31:48Z

iceoryx_posh/source/roudi/port_manager.cpp

+    // delete client port from list after DISCONNECT was processed
+    m_portPool->removeClientPort(clientPortData);
+
+    LogDebug() << "Destroyed client port";


I would think it maybe helpful to add the process name and service description here.

Unfortunately the process name is not available at this point, but I added the service description

elfenpiff · 2022-02-11T09:32:50Z

iceoryx_posh/source/roudi/port_manager.cpp

+            /// @todo iox-#27 report to port introspection
+            if (!this->sendToAllMatchingServerPorts(caproMessage, clientPort))
+            {
+                LogDebug() << "capro::CONNECT/DISCONNECT, no matching server!!";


Could you add the process name of the port owner and the service description? This goes also for all the other log messages.

same as above, the process name is not available ... or did you mean the runtime name?

iceoryx_posh/source/roudi/port_manager.cpp

iceoryx_posh/source/roudi/port_pool.cpp

codecov · 2022-02-14T15:40:31Z

Codecov Report

Merging #1084 (4a550b8) into master (d686972) will increase coverage by 0.14%.
The diff coverage is 82.06%.

@@            Coverage Diff             @@
##           master    #1084      +/-   ##
==========================================
+ Coverage   77.23%   77.38%   +0.14%     
==========================================
  Files         346      346              
  Lines       13159    13389     +230     
  Branches     1884     1917      +33     
==========================================
+ Hits        10164    10361     +197     
- Misses       2371     2395      +24     
- Partials      624      633       +9

Flag	Coverage Δ
unittests	`76.62% <82.06%> (+0.14%)`	⬆️
unittests_timing	`12.20% <0.00%> (-0.20%)`	⬇️

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

Impacted Files	Coverage Δ
...include/iceoryx_posh/capro/service_description.hpp	`100.00% <ø> (ø)`
...ryx_posh/internal/popo/ports/client_port_roudi.hpp	`100.00% <ø> (ø)`
...ryx_posh/internal/popo/ports/server_port_roudi.hpp	`100.00% <ø> (ø)`
...clude/iceoryx_posh/internal/roudi/port_manager.hpp	`100.00% <ø> (ø)`
...oryx_posh/include/iceoryx_posh/roudi/port_pool.hpp	`100.00% <ø> (ø)`
iceoryx_posh/source/roudi/port_manager.cpp	`80.53% <77.61%> (+0.11%)`	⬆️
...ude/iceoryx_posh/internal/roudi/port_pool_data.inl	`96.15% <100.00%> (ø)`
iceoryx_posh/source/capro/service_description.cpp	`98.03% <100.00%> (+0.08%)`	⬆️
...ceoryx_posh/source/popo/ports/client_port_data.cpp	`100.00% <100.00%> (ø)`
...eoryx_posh/source/popo/ports/client_port_roudi.cpp	`95.04% <100.00%> (+0.08%)`	⬆️
... and 8 more

iceoryx_posh/source/roudi/port_manager.cpp

elBoberido · 2022-02-16T23:12:30Z

iceoryx_posh/test/moduletests/test_roudi_portpool.cpp

+
+// BEGIN ClientPort tests
+
+TEST_F(PortPool_test, AddClientPortIsSuccessful)


I'm not that happy with the naming but all tests in this file follow the same schema and this makes it easier to check if all ports have the same tests

iceoryx_posh/include/iceoryx_posh/capro/service_description.hpp

iceoryx_posh/source/capro/service_description.cpp

iceoryx_posh/source/popo/ports/client_port_data.cpp

iceoryx_posh/source/roudi/port_manager.cpp

iceoryx_posh/source/popo/ports/client_port_data.cpp

…erverPortData

… create a new one

… the policy

…erface ports

elfenpiff

Please substitute elfenpiff with budrus :D

iceoryx_posh/test/moduletests/test_capro_service.cpp

elBoberido added the enhancement New feature label Feb 9, 2022

elBoberido self-assigned this Feb 9, 2022

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch 3 times, most recently from 6677cc1 to c1dbf60 Compare February 10, 2022 12:46

elBoberido force-pushed the iox-#27-consolidate-port-queue-policies branch from dd30a28 to b17164e Compare February 10, 2022 13:40

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from c1dbf60 to 265ef74 Compare February 10, 2022 13:40

elBoberido force-pushed the iox-#27-consolidate-port-queue-policies branch from b17164e to e4e90b6 Compare February 10, 2022 15:09

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 265ef74 to e35218f Compare February 10, 2022 15:10

elBoberido force-pushed the iox-#27-consolidate-port-queue-policies branch from e4e90b6 to c0894b5 Compare February 10, 2022 15:22

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from e35218f to a9ddacd Compare February 10, 2022 15:22

elBoberido force-pushed the iox-#27-consolidate-port-queue-policies branch from c0894b5 to 84a0fe5 Compare February 10, 2022 16:26

elBoberido changed the title ~~[do-not-review] iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2]~~ iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] Feb 10, 2022

elBoberido linked an issue Feb 10, 2022 that may be closed by this pull request

Request/Response communication with iceoryx #27

Closed

22 tasks

elfenpiff reviewed Feb 11, 2022

View reviewed changes

elfenpiff mentioned this pull request Feb 11, 2022

Iox #27 consolidate port queue policies [stacked PR #1] #1075

Merged

21 tasks

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch 2 times, most recently from 3c9d622 to 1a53faa Compare February 11, 2022 11:19

Base automatically changed from iox-#27-consolidate-port-queue-policies to master February 14, 2022 14:28

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 9372d15 to d10b138 Compare February 14, 2022 15:20

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 34bc993 to 3738316 Compare February 15, 2022 18:44

elBoberido mentioned this pull request Feb 15, 2022

iox-#27 Refactor CaproMessageSubType to be able to use ServerPorts with InterfacePorts [stacked PR #1.5] #1114

Merged

21 tasks

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from 3738316 to 2b7e9f4 Compare February 16, 2022 17:49

elBoberido commented Feb 16, 2022

View reviewed changes

iceoryx_posh/source/roudi/port_manager.cpp Show resolved Hide resolved

elBoberido commented Feb 16, 2022

View reviewed changes

budrus self-requested a review February 17, 2022 10:59

budrus reviewed Feb 17, 2022

View reviewed changes

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch 2 times, most recently from e8a28b5 to 1f4c22b Compare February 17, 2022 21:57

elBoberido added 21 commits February 18, 2022 17:29

iox-#27 Propagate memoryInfo to ChunkReceiver of ClientPortData and S…

5f610c9

…erverPortData

iox-#27 Add const

d6b796b

iox-#27 Add PortPool client tests

32b2648

iox-#27 Add PortPool server tests

b0dc5df

iox-#27 Add doxygen documentation

d51c019

iox-#27 Release all chunks of the port only after beeing inactive

95e91d0

iox-#27 Clarify comments

2546ec0

iox-#27 Check if there is an outdated server port before rejecting to…

a16d520

… create a new one

iox-#27 Split test fixture of PortManager_test into separate hpp file

fa5bd4c

iox-#27 Add PortManager tests to acquire clients and servers

3740311

iox-#27 Add PortManager tests for client and server discovery

f462314

iox-#27 Add PortManager tests for client and server connection policy

ebf207a

iox-#27 Make all client and server policies available

70e746d

iox-#27 Connect only client and server which are compatible according…

97e6630

… the policy

iox-#27 Add PortManager test for client and server communication

56768ff

iox-#27 Register server at the service registry

fe2ff90

iox-#27 Use separate issue number for introspection to-dos

4439fa5

iox-#27 Add PortManager tests for client and server forwarding to int…

4243c6a

…erface ports

iox-#27 Early return after capro protocol error

985b036

iox-#27 Remove IOX_MAYBE_UNUSED

9eea005

iox-#27 Better naming for test variables

5a1bf33

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from a68ff3e to d2c13c1 Compare February 18, 2022 16:30

elfenpiff requested changes Feb 18, 2022

View reviewed changes

iceoryx_posh/test/moduletests/test_capro_service.cpp Outdated Show resolved Hide resolved

iox-#27 Multi character service description for logger test

4a550b8

elBoberido force-pushed the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch from d2c13c1 to 4a550b8 Compare February 18, 2022 18:17

elBoberido requested a review from elfenpiff February 18, 2022 18:18

elfenpiff approved these changes Feb 20, 2022

View reviewed changes

budrus approved these changes Feb 20, 2022

View reviewed changes

elBoberido merged commit ca6fa0f into master Feb 20, 2022

elBoberido deleted the iox-#27-add-client-and-server-port-to-port-pool-and-port-manager branch February 20, 2022 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] #1084

iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] #1084

elBoberido commented Feb 9, 2022 •

edited

Loading

elfenpiff left a comment

elfenpiff Feb 11, 2022

elBoberido Feb 11, 2022

elfenpiff Feb 11, 2022

elBoberido Feb 11, 2022

elfenpiff Feb 11, 2022

elBoberido Feb 11, 2022

codecov bot commented Feb 14, 2022 •

edited

Loading

elBoberido Feb 16, 2022

elfenpiff left a comment

		@@ -40,7 +40,7 @@ typedef struct
		bool offerOnCreate;

		/// @brief describes whether a publisher blocks when subscriber queue is full


		// BEGIN ClientPort tests

		TEST_F(PortPool_test, AddClientPortIsSuccessful)

iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] #1084

iox-#27 Add client and server port to PortPool and PortManager [stacked PR #2] #1084

Conversation

elBoberido commented Feb 9, 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

elfenpiff left a comment

Choose a reason for hiding this comment

elfenpiff Feb 11, 2022

Choose a reason for hiding this comment

elBoberido Feb 11, 2022

Choose a reason for hiding this comment

elfenpiff Feb 11, 2022

Choose a reason for hiding this comment

elBoberido Feb 11, 2022

Choose a reason for hiding this comment

elfenpiff Feb 11, 2022

Choose a reason for hiding this comment

elBoberido Feb 11, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

elBoberido Feb 16, 2022

Choose a reason for hiding this comment

elfenpiff left a comment

Choose a reason for hiding this comment

elBoberido commented Feb 9, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading