Iox #27 server port implementation preparatory #894

elBoberido · 2021-08-02T19:02:06Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This is a preparatory PR for the server port implementation in order to keep that PR small.

The ChunkSender, ChunkDistributor and RpcHeader are extended with the functionality needed by the ServerPort. Additionally, the ServerPortOptions are added.

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

Partly closes Request/Response communication with iceoryx #27

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.hpp

iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp

codecov · 2022-01-06T16:10:42Z

Codecov Report

Merging #894 (7513b83) into master (b45d65e) will increase coverage by 0.05%.
The diff coverage is 96.72%.

@@            Coverage Diff             @@
##           master     #894      +/-   ##
==========================================
+ Coverage   76.96%   77.01%   +0.05%     
==========================================
  Files         342      343       +1     
  Lines       12425    12475      +50     
  Branches     1845     1856      +11     
==========================================
+ Hits         9563     9608      +45     
- Misses       2242     2243       +1     
- Partials      620      624       +4

Flag	Coverage Δ
unittests	`76.24% <96.72%> (+0.07%)`	⬆️
unittests_timing	`12.28% <0.00%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
...nternal/popo/building_blocks/chunk_distributor.hpp	`100.00% <ø> (ø)`
...osh/internal/popo/building_blocks/chunk_sender.hpp	`100.00% <ø> (ø)`
...nternal/popo/building_blocks/chunk_distributor.inl	`92.06% <93.75%> (+0.22%)`	⬆️
...de/iceoryx_posh/internal/mepoo/segment_manager.inl	`86.66% <100.00%> (-1.47%)`	⬇️
...osh/internal/popo/building_blocks/chunk_sender.inl	`90.47% <100.00%> (+1.00%)`	⬆️
...ceoryx_posh/source/popo/ports/client_port_user.cpp	`96.22% <100.00%> (ø)`
iceoryx_posh/source/popo/rpc_header.cpp	`100.00% <100.00%> (ø)`
iceoryx_posh/source/popo/server_options.cpp	`100.00% <100.00%> (ø)`
iceoryx_hoofs/source/posix_wrapper/timer.cpp	`62.44% <0.00%> (-0.85%)`	⬇️
... and 1 more

elfenpiff · 2022-01-18T15:31:30Z

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.hpp

+    /// @param[in] uniqueQueueId is an unique ID which identifies the queue to which this chunk shall be delivered
+    /// @param[in] lastKnownQueueIndex is used for a fast lookup of the queue with uniqueQueueId
+    /// @param[in] chunk is the SharedChunk to be delivered
+    /// @return ChunkDistributorError if the queue was not found


What happens when to queue would overflow? Can this happen?

Of course. It can also happen that the call is blocking. This all depends on the SubscriberTooSlowPolicy and QueueFullPolicy

elfenpiff · 2022-01-18T15:34:03Z

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender.hpp

+    /// @param[in] lastKnownQueueIndex is used for a fast lookup of the queue with uniqueQueueId
+    /// @return true when successful, false otherwise
+    /// @note This method does not add the chunk to the history
+    bool sendToQueue(mepoo::ChunkHeader* const chunkHeader,


Is it valid to have the nullptr as argument for chunkHeader? If not I would change the chunkHeader argument into a reference to communicate this to the user that this is never a nullptr.

The same argument goes also for the other methods but you did not touch the fishy, but if you touch the fishy you can turn the pointer into a reference.

https://www.youtube.com/watch?v=8gdsueMGUfk

Well, it is the same behavior as with ChunkSender::send. One can argue if that is the desired behavior but since that can easily becomes a bottomless pit I would argue it should be discussed/implemented in a separate issue.

elfenpiff · 2022-01-18T15:38:59Z

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_sender.inl

+{
+    mepoo::SharedChunk chunk(nullptr);
+    // BEGIN of critical section, chunk will be lost if the process terminates in this section
+    if (getChunkReadyForSend(chunkHeader, chunk))


What happens when chunkHeader == nullptr?

Then getChunkReadyForSend will return false and the error handler will be called

elfenpiff · 2022-01-18T15:41:38Z

iceoryx_posh/source/popo/server_options.cpp

+        serverOptions.requestQueueCapacity, serverOptions.nodeName, serverOptions.offerOnCreate, clientTooSlowPolicy);
+
+    if (!deserializationSuccessful
+        || clientTooSlowPolicy > static_cast<ClientTooSlowPolicyUT>(ConsumerTooSlowPolicy::DISCARD_OLDEST_DATA))


I prefer the approach of adding an additional enum called END and then one can add additional enum values without touching any piece of code.

iceoryx_posh/test/moduletests/test_popo_chunk_distributor.cpp

…ueue

…alization

elBoberido commented Aug 2, 2021

View reviewed changes

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.hpp Outdated Show resolved Hide resolved

elBoberido commented Aug 2, 2021

View reviewed changes

iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp Outdated Show resolved Hide resolved

elBoberido self-assigned this Sep 13, 2021

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch from 6503b29 to c7bebf4 Compare September 13, 2021 21:53

This was referenced Jan 6, 2022

Process scoped unique ID #1010

Closed

Iox #1010 unique id class #1011

Merged

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch from c7bebf4 to 908deea Compare January 6, 2022 15:45

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch 5 times, most recently from 4528310 to 0c86a01 Compare January 7, 2022 17:54

elBoberido requested review from elfenpiff, FerdinandSpitzschnueffler and MatthiasKillat and removed request for elfenpiff, MatthiasKillat and FerdinandSpitzschnueffler January 7, 2022 17:54

elBoberido changed the title ~~Iox #27 server port implementation preparatory~~ [do not review before #1011 is merged] Iox #27 server port implementation preparatory Jan 7, 2022

elBoberido marked this pull request as ready for review January 7, 2022 18:01

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch 4 times, most recently from b788f15 to 4fa3da6 Compare January 11, 2022 13:28

elBoberido changed the title ~~[do not review before #1011 is merged] Iox #27 server port implementation preparatory~~ Iox #27 server port implementation preparatory Jan 11, 2022

elBoberido requested review from elfenpiff, FerdinandSpitzschnueffler and MatthiasKillat January 11, 2022 13:32

elBoberido added the enhancement New feature label Jan 11, 2022

elfenpiff reviewed Jan 18, 2022

View reviewed changes

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch from 1b697dc to 2473871 Compare January 18, 2022 17:13

elBoberido requested review from elfenpiff and FerdinandSpitzschnueffler January 19, 2022 15:21

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch 2 times, most recently from 16347c7 to 1d4c1cd Compare January 20, 2022 10:15

FerdinandSpitzschnueffler reviewed Jan 20, 2022

View reviewed changes

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

elBoberido added 16 commits January 20, 2022 13:58

iox-#27 Add ServerOptions

2a066c6

iox-#27 Add UniqueId to ChunkQueueData

374bd51

iox-#27 Use cxx::UniqueId in RpcHeader

396c28d

iox-#27 Extend ChunkDistributor and ChunkSender to send to specific q…

e67a800

…ueue

iox-#27 Add tests for ServerOptions

dbdac7c

iox-#27 Implement getQueueIndex and deliverToQueue in ChunkDistributor

075759c

iox-#27 Make the underlying type of enums unsigned to simplify deseri…

3fbf221

…alization

iox-#27 Add tests for ChunkDistributor::getQueueIndex

6d7ef7b

iox-#27 Add tests for ChunkDistributor::deliverToQueue

65984f4

iox-#27 Rename overloaded deliverToQueue method to pushToQueue

0a4f9ce

iox-#27 Add tests and documenation to ChunkSender::sendToQueue

a7640dc

iox-#27 Disable a death test for macOS

56a006d

iox-#27 Fix typo, doxygen and better wording

be7ea84

iox-#27 Add test to get index of removed queue

ec5922a

iox-#27 Add test IDs for new tests

c7cb310

iox-#27 Fix fatal LSAN error

7513b83

elBoberido force-pushed the iox-#27-server-port-implementation-preparatory branch from 1d4c1cd to 7513b83 Compare January 20, 2022 12:59

elBoberido requested a review from FerdinandSpitzschnueffler January 20, 2022 12:59

FerdinandSpitzschnueffler approved these changes Jan 20, 2022

View reviewed changes

elfenpiff approved these changes Jan 21, 2022

View reviewed changes

elBoberido merged commit dfb6857 into eclipse-iceoryx:master Jan 21, 2022

elBoberido deleted the iox-#27-server-port-implementation-preparatory branch January 21, 2022 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #27 server port implementation preparatory #894

Iox #27 server port implementation preparatory #894

elBoberido commented Aug 2, 2021 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

elfenpiff Jan 18, 2022

elBoberido Jan 18, 2022

elfenpiff Jan 18, 2022

elBoberido Jan 18, 2022

elfenpiff Jan 18, 2022

elBoberido Jan 18, 2022

elfenpiff Jan 18, 2022

Iox #27 server port implementation preparatory #894

Iox #27 server port implementation preparatory #894

Conversation

elBoberido commented Aug 2, 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 Jan 6, 2022 • edited Loading

Codecov Report

elfenpiff Jan 18, 2022

Choose a reason for hiding this comment

elBoberido Jan 18, 2022

Choose a reason for hiding this comment

elfenpiff Jan 18, 2022

Choose a reason for hiding this comment

elBoberido Jan 18, 2022

Choose a reason for hiding this comment

elfenpiff Jan 18, 2022

Choose a reason for hiding this comment

elBoberido Jan 18, 2022

Choose a reason for hiding this comment

elfenpiff Jan 18, 2022

Choose a reason for hiding this comment

elBoberido commented Aug 2, 2021 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading