Feature/tcp admin msg segmentation #279

rbulter · 2020-08-29T08:55:03Z

No description provided.

- Fix pre-allocated buffer handling - Solve message corruption because of wrong locking in write function - Add interceptors for tcp_admin

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c

merge master

- Change endpoint handling

lower wait times

…sg_segmentation

codecov-io · 2020-10-06T19:13:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@20f794b). Click here to learn what that means.
The diff coverage is 64.01%.

@@            Coverage Diff            @@
##             master     #279   +/-   ##
=========================================
  Coverage          ?   68.61%           
=========================================
  Files             ?      137           
  Lines             ?    27382           
  Branches          ?        0           
=========================================
  Hits              ?    18788           
  Misses            ?     8594           
  Partials          ?        0

Impacted Files	Coverage Δ
...les/pubsub/pubsub_admin_tcp/src/pubsub_tcp_admin.c	`51.74% <20.00%> (ø)`
...es/pubsub/pubsub_admin_tcp/src/pubsub_tcp_common.c	`40.00% <40.00%> (ø)`
bundles/pubsub/pubsub_utils/src/pubsub_utils_url.c	`51.75% <54.54%> (ø)`
...b/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c	`61.60% <58.33%> (ø)`
...s/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c	`75.13% <65.51%> (ø)`
...sub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c	`67.06% <80.95%> (ø)`
...undles/pubsub/pubsub_admin_tcp/src/psa_activator.c	`100.00% <100.00%> (ø)`
...sub_topology_manager/src/pubsub_topology_manager.c	`57.60% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20f794b...17c6f7c. Read the comment docs.

codecov-commenter · 2020-10-06T19:15:32Z

Codecov Report

Merging #279 into master will increase coverage by 0.22%.
The diff coverage is 64.01%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   68.30%   68.53%   +0.22%     
==========================================
  Files         136      137       +1     
  Lines       27371    27443      +72     
==========================================
+ Hits        18696    18808     +112     
+ Misses       8675     8635      -40

Impacted Files	Coverage Δ
...les/pubsub/pubsub_admin_tcp/src/pubsub_tcp_admin.c	`48.47% <20.00%> (-0.54%)`	⬇️
...es/pubsub/pubsub_admin_tcp/src/pubsub_tcp_common.c	`40.00% <40.00%> (ø)`
bundles/pubsub/pubsub_utils/src/pubsub_utils_url.c	`51.75% <54.54%> (ø)`
...b/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c	`61.60% <58.33%> (-1.66%)`	⬇️
...s/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c	`72.51% <65.51%> (-2.74%)`	⬇️
...sub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c	`67.06% <80.95%> (+2.71%)`	⬆️
...undles/pubsub/pubsub_admin_tcp/src/psa_activator.c	`100.00% <100.00%> (ø)`
...sub_topology_manager/src/pubsub_topology_manager.c	`57.48% <100.00%> (+4.68%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20f794b...17c6f7c. Read the comment docs.

codecov-io · 2020-10-06T19:42:57Z

Codecov Report

Merging #279 (9663481) into master (dd4dca1) will increase coverage by 0.09%.
The diff coverage is 63.65%.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   68.17%   68.26%   +0.09%     
==========================================
  Files         148      149       +1     
  Lines       30088    30171      +83     
==========================================
+ Hits        20511    20597      +86     
+ Misses       9577     9574       -3

Impacted Files	Coverage Δ
...es/pubsub/pubsub_admin_tcp/src/pubsub_tcp_common.c	`0.00% <0.00%> (ø)`
...les/pubsub/pubsub_admin_tcp/src/pubsub_tcp_admin.c	`48.24% <20.00%> (-0.76%)`	⬇️
...b/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c	`63.47% <50.70%> (+0.20%)`	⬆️
bundles/pubsub/pubsub_utils/src/pubsub_utils_url.c	`53.07% <54.54%> (ø)`
...sub_topology_manager/src/pubsub_topology_manager.c	`54.25% <60.00%> (+1.34%)`	⬆️
...s/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c	`75.42% <67.34%> (+0.18%)`	⬆️
...sub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c	`68.93% <84.05%> (+2.77%)`	⬆️
...undles/pubsub/pubsub_admin_tcp/src/psa_activator.c	`100.00% <100.00%> (ø)`
...b_protocol_wire_v1/src/pubsub_wire_protocol_impl.c	`96.38% <100.00%> (+9.80%)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd4dca1...9663481. Read the comment docs.

Merge Master

rbulter

I have updated the review comments

Oipo · 2020-12-14T10:01:31Z

@rbulter you have missed some review comments. Please check them all.

rbulter

Done

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c

bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.h

Oipo

Still plenty of changes requested left, some from previous review. Cannot approve this PR until those have been resolved.

pnoltes · 2020-12-17T14:42:09Z

Our IVV test where failing for the updated PubSub TCP Admin, apparently this update does not work with wire protocol v1.

@rbulter Is it correct that the updated PubSub TCP Admin does not work with wire protocol v1?
If not, please add a pubsub tcp admin test with wire protocol v1.
If correct, please update the tcp admin match , so that only wire protocol v2 is searched and supported.

Maybe add an pubsubEndpoint_matchPublisherWithRequestedProtocolAndSerializer function which is used by the 'normal' pubsubEndpoint_matchPublisher and can be called directly by the tcp admin match with a filled in requestedProtcol="envelope-v2" argument. And the same for pubsubEnd_matchSubscriber.
This should enforce the tcp admin to use the correct wire protocol or fail in the match.

pnoltes

see comment about wire protocol

rbulter · 2020-12-17T19:24:57Z

The pubsub admin should work with wire protocol v1, in the past it was working. I will add a unit test

rbulter · 2020-12-17T19:26:26Z

I added a test for wire_v1 and found the error in wire_v1. During the refactor of wire_v1 a flag was removed, which is needed by tcp_admin for message segmentation

Oipo

IVV tests are green with the latest changes. The only thing left now is to merge develop into this branch, as that will solve the conflicts and fix the OSX build.

Thank you for the effort put into fixing review comments.

Oipo · 2020-12-16T16:33:20Z

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.h

 bool pubsub_tcpTopicSender_isPassive(pubsub_tcp_topic_sender_t *sender);
 long pubsub_tcpTopicSender_serializerSvcId(pubsub_tcp_topic_sender_t *sender);
 long pubsub_tcpTopicSender_protocolSvcId(pubsub_tcp_topic_sender_t *sender);
+/* Note this functions are deprecated and not used */


Oipo · 2020-12-16T16:33:58Z

bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c

    if ( handlingThreadSleepTime >= 0 ) {
        manager->handlingThreadSleepTime = handlingThreadSleepTime * 1000L;
    }
+    manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_MS_KEY,  manager->handlingThreadSleepTime);


rbulter added 3 commits August 23, 2020 20:56

- Add Message segmenation (maxSize / IOVEC)

90f472e

- Fix pre-allocated buffer handling - Solve message corruption because of wrong locking in write function - Add interceptors for tcp_admin

Fix tests

954d64d

undo test change

3df62e7

Oipo reviewed Aug 29, 2020

View reviewed changes

bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c Outdated Show resolved Hide resolved

rbulter added 10 commits August 30, 2020 20:52

Merge pull request #5 from apache/master

9f6c8a2

merge master

- Fix connection loss when receive buffer is not filled

56ec4b1

- Change endpoint handling

- Revert RELEASE_NOTES change

00e9c19

Fix Segfault when no url is selected

6363c7a

Merge branch 'master' into feature/tcp_admin_msg_segmentation

b83b186

Remove mem lead in read handler

b127dd6

remove non blocking read

fd2ea24

fix endpoint test

c0fe056

lower wait times

Fix locking

0dc5867

Merge remote-tracking branch 'origin/master' into feature/tcp_admin_m…

17c6f7c

…sg_segmentation

Fix tests

baeda66

rbulter added 5 commits October 6, 2020 21:51

Fix Cmake file

3b95016

Fix mem leak

e03c89b

Remove warning

7ef5e0e

Fix memleak

da1030d

Refactor read function

33db85b

rbulter requested a review from pnoltes October 16, 2020 12:14

rbulter added 6 commits October 16, 2020 14:42

Merge branch 'master' of https://github.com/apache/celix

67fa366

Merge pull request #6 from apache/master

306dd79

Merge Master

Merge branch 'master' of https://github.com/rbulter/celix

e72ab1c

Update for performance

000245e

Fix headerless

0492d9f

Include header in maxMsgSize

df86737

rbulter commented Dec 2, 2020

View reviewed changes

rbulter added 2 commits December 2, 2020 21:22

Move test option

d712ee5

- Move test options

5e447f9

rbulter requested a review from Oipo December 14, 2020 14:32

Fix Small things

e94b036

rbulter commented Dec 14, 2020

View reviewed changes