[12552] delete_contained_entities implementation #2223

jsan-rt · 2021-09-23T07:57:28Z

Added delete_contained_entities functionality to DomainParticipant, Publisher, Subscriber and DataReader.

…Participant, Publisher, Subscriber and DataReader Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

…s implementation Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

…ent function Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo · 2021-10-05T06:49:29Z

src/cpp/fastdds/subscriber/SubscriberImpl.cpp

+    std::vector<DataReader*> reader_vector;
+    get_datareaders(reader_vector);
+
+    std::lock_guard<std::mutex> lock(mtx_readers_);


Move the lock up, to ensure there is no other reader created in between

Suggested change

std::vector<DataReader*> reader_vector;

get_datareaders(reader_vector);

std::lock_guard<std::mutex> lock(mtx_readers_);

std::lock_guard<std::mutex> lock(mtx_readers_);

std::vector<DataReader*> reader_vector;

get_datareaders(reader_vector);

Also, we can traverse the readers_ map directly, and we'll avoid allocating the reader_vector elements on the heap.

IkerLuengo · 2021-10-05T06:50:29Z

src/cpp/fastdds/subscriber/SubscriberImpl.cpp

+    for (DataReader* reader: reader_vector)
+    {
+        can_be_deleted = can_be_deleted && reader->impl_->can_be_deleted();
+        if (!can_be_deleted)
+        {
+            return ReturnCode_t::RETCODE_PRECONDITION_NOT_MET;
+        }
+    }


We can avoid the result variable altogether

Suggested change

for (DataReader* reader: reader_vector)

{

can_be_deleted = can_be_deleted && reader->impl_->can_be_deleted();

if (!can_be_deleted)

{

return ReturnCode_t::RETCODE_PRECONDITION_NOT_MET;

}

}

for (DataReader* reader: reader_vector)

{

if (!reader->impl_->can_be_deleted())

{

return ReturnCode_t::RETCODE_PRECONDITION_NOT_MET;

}

}

IkerLuengo · 2021-10-05T07:06:24Z

src/cpp/fastdds/subscriber/SubscriberImpl.cpp

+            return_status = return_status && dr->can_be_deleted();
+            if (!return_status)
+            {
+                return return_status;
+            }


We can simplify and avoid the bitwise AND.
Same applies to the publisher

Suggested change

return_status = return_status && dr->can_be_deleted();

if (!return_status)

{

return return_status;

}

return_status = dr->can_be_deleted();

if (!return_status)

{

return true;

}

IkerLuengo · 2021-10-05T07:07:00Z

src/cpp/fastdds/publisher/PublisherImpl.cpp

+    //std::vector<DataWriter*> writer_vector;
+    //get_datawriters(writer_vector);


Remove commented code

IkerLuengo · 2021-10-05T07:07:23Z

src/cpp/fastdds/publisher/PublisherImpl.cpp

+    /*  std::lock_guard<std::mutex> lock(mtx_writers_);
+       for (DataWriter* writer: writer_vector)
+       {
+          can_be_deleted = can_be_deleted && (writer->impl_->check_delete_preconditions() == ReturnCode_t::RETCODE_OK);
+          if (!can_be_deleted)
+          {
+              return can_be_deleted;
+          }
+       }*/


Remove commented code

IkerLuengo · 2021-10-05T07:20:36Z

src/cpp/fastdds/domain/DomainParticipantImpl.cpp

+
+        for (auto& subscriber : subscribers_)
+        {
+            subscriber.first->delete_contained_entities();


Check the return code and exit with error if it is not OK

IkerLuengo · 2021-10-05T07:20:59Z

src/cpp/fastdds/domain/DomainParticipantImpl.cpp

+
+        for (auto& publisher : publishers_)
+        {
+            publisher.first->delete_contained_entities();


Check the return code and exit with error if it is not OK

IkerLuengo · 2021-10-05T07:45:16Z

test/unittest/dds/subscriber/DataReaderTests.cpp

+        );
+    ASSERT_EQ(query_condition, nullptr);
+
+    ASSERT_EQ(data_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);


Maybe add a comment stating that this must be updated once the query conditions are in place

IkerLuengo · 2021-10-05T07:45:31Z

test/unittest/dds/subscriber/SubscriberTests.cpp

+    ASSERT_TRUE(data_reader_list.size() == 2);
+
+    data_reader_list.clear();
+    ASSERT_EQ(subscriber->delete_contained_entities(), ReturnCode_t::RETCODE_OK);


Maybe add a comment stating that this must be updated once the query conditions are in place

IkerLuengo · 2021-10-05T07:52:00Z

test/mock/dds/DomainParticipantImpl/fastdds/domain/DomainParticipantImpl.hpp

@@ -535,6 +535,83 @@ class DomainParticipantImpl
        return false;
    }

+    ReturnCode_t delete_contained_entities()


Update this implementation to manage the locks as in the real implementation. It is not so important in the mock, but releasing the locks may result in publishers or subscribers being create in between.

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo · 2021-10-05T14:37:51Z

src/cpp/fastdds/publisher/PublisherImpl.cpp

+    //std::vector<DataWriter*> writer_vector;
+    //get_datawriters(writer_vector);


Remove commented code

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo

CI errors seem to be unrelated to the changes.
LGTM

MiguelCompany · 2021-11-22T07:40:41Z

@Mergifyio backport 2.3.x

* Refs #12532: Added delete_contained_entities implementation to DomainParticipant, Publisher, Subscriber and DataReader Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added tests to validate the new delete_contained_entities implementation Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Formatting and comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified test to check for entity presence with a different function Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added delete_contained_entities to Statistics mock classes Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified locking logic and unit test Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: General improvements from PR suggestions Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> (cherry picked from commit af0ff68) # Conflicts: # test/unittest/dds/subscriber/DataReaderTests.cpp # test/unittest/dds/subscriber/SubscriberTests.cpp

mergify · 2021-11-22T07:41:24Z

backport 2.3.x

✅ Backports have been created

#2322 [12552] delete_contained_entities implementation (backport #2223) has been created for branch 2.3.x

* Refs #12532: Added delete_contained_entities implementation to DomainParticipant, Publisher, Subscriber and DataReader Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added tests to validate the new delete_contained_entities implementation Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Formatting and comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified test to check for entity presence with a different function Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added delete_contained_entities to Statistics mock classes Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified locking logic and unit test Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: General improvements from PR suggestions Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> (cherry picked from commit af0ff68)

* delete_contained_entities implementation (#2223) * Refs #12532: Added delete_contained_entities implementation to DomainParticipant, Publisher, Subscriber and DataReader Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added tests to validate the new delete_contained_entities implementation Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Formatting and comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified test to check for entity presence with a different function Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Added delete_contained_entities to Statistics mock classes Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Modified locking logic and unit test Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: General improvements from PR suggestions Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> * Refs #12532: Comment cleanup Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> (cherry picked from commit af0ff68) * Refs #12990: delete_contained_entities 2.3.x backport Signed-off-by: Javier Santiago <javiersantiago@eprosima.com> Co-authored-by: jsantiago-eProsima <90755661+jsantiago-eProsima@users.noreply.github.com> Co-authored-by: Javier Santiago <javiersantiago@eprosima.com>

jsan-rt added 3 commits September 22, 2021 16:19

Refs #12532: Added delete_contained_entities implementation to Domain…

b6ea874

…Participant, Publisher, Subscriber and DataReader Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Refs #12532: Added tests to validate the new delete_contained_entitie…

4bda63b

…s implementation Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Refs #12532: Formatting and comment cleanup

2db0ac1

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

jsan-rt marked this pull request as ready for review September 30, 2021 08:36

jsan-rt added 3 commits September 30, 2021 15:05

Refs #12532: Modified test to check for entity presence with a differ…

d7d239a

…ent function Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Refs #12532: Added delete_contained_entities to Statistics mock classes

8f91dfd

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

Refs #12532: Modified locking logic and unit test

1faf38d

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo suggested changes Oct 5, 2021

View reviewed changes

Refs #12532: General improvements from PR suggestions

1d2decd

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo suggested changes Oct 5, 2021

View reviewed changes

Refs #12532: Comment cleanup

c8dbfa5

Signed-off-by: Javier Santiago <javiersantiago@eprosima.com>

IkerLuengo approved these changes Oct 6, 2021

View reviewed changes

MiguelCompany merged commit af0ff68 into master Oct 6, 2021

MiguelCompany deleted the feature/delete_contained_entities branch October 6, 2021 13:51

mergify bot mentioned this pull request Nov 22, 2021

[12552] delete_contained_entities implementation (backport #2223) #2322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12552] delete_contained_entities implementation #2223

[12552] delete_contained_entities implementation #2223

jsan-rt commented Sep 23, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo Oct 5, 2021

IkerLuengo left a comment

MiguelCompany commented Nov 22, 2021

mergify bot commented Nov 22, 2021

		//std::vector<DataWriter*> writer_vector;
		//get_datawriters(writer_vector);

[12552] delete_contained_entities implementation #2223

[12552] delete_contained_entities implementation #2223

Conversation

jsan-rt commented Sep 23, 2021

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

IkerLuengo left a comment

Choose a reason for hiding this comment

MiguelCompany commented Nov 22, 2021

mergify bot commented Nov 22, 2021

✅ Backports have been created