Skip to content

Commit

Permalink
Just show warning when inconsistency between depth and max_samples_pe…
Browse files Browse the repository at this point in the history 
…r_instance (#4417)

* Refs #20503. Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20503. Show warning when depth > max_samples_per_instance

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20503. Fix InvalidQos tests

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Co-authored-by: EduPonz <eduardoponz@eprosima.com>
  • Loading branch information
@MiguelCompany @EduPonz @Mario-DL
2 people authored and Mario-DL committed Mar 19, 2024
1 parent 614ac65 commit c29020b
Show file tree
Hide file tree
Showing 6 changed files with 197 additions and 24 deletions.
7 changes: 5 additions & 2 deletions src/cpp/fastdds/publisher/DataWriterImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1713,8 +1713,11 @@ ReturnCode_t DataWriterImpl::check_qos(
qos.resource_limits().max_samples_per_instance > 0 &&
qos.history().depth > qos.resource_limits().max_samples_per_instance)
{
logError(RTPS_QOS_CHECK,"HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
logWarning(RTPS_QOS_CHECK,
"HISTORY DEPTH '" << qos.history().depth <<
"' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
"'. Consistency rule: depth <= max_samples_per_instance." <<
" Effectively using max_samples_per_instance as depth.");
}
return ReturnCode_t::RETCODE_OK;
}
Expand Down
7 changes: 5 additions & 2 deletions src/cpp/fastdds/subscriber/DataReaderImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1330,8 +1330,11 @@ ReturnCode_t DataReaderImpl::check_qos (
qos.resource_limits().max_samples_per_instance > 0 &&
qos.history().depth > qos.resource_limits().max_samples_per_instance)
{
logError(RTPS_QOS_CHECK, "HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
logWarning(RTPS_QOS_CHECK,
"HISTORY DEPTH '" << qos.history().depth <<
"' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
"'. Consistency rule: depth <= max_samples_per_instance." <<
" Effectively using max_samples_per_instance as depth.");
}
return ReturnCode_t::RETCODE_OK;
}
Expand Down
11 changes: 11 additions & 0 deletions test/blackbox/common/DDSBlackboxTestsDataReader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,17 @@ TEST(DDSDataReader, ConsistentReliabilityWhenIntraprocess)
xmlparser::XMLProfileManager::library_settings(library_settings);
}

/**
* This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
* It checks that a DataReader be created with default Qos and a large history depth.
*/
TEST(DDSDataReader, default_qos_large_history_depth)
{
PubSubReader<HelloWorldPubSubType> reader(TEST_TOPIC_NAME);
reader.history_depth(1000).init();
ASSERT_TRUE(reader.isInitialized());
}

#ifdef INSTANTIATE_TEST_SUITE_P
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
#else
Expand Down
11 changes: 11 additions & 0 deletions test/blackbox/common/DDSBlackboxTestsDataWriter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,17 @@ TEST(DDSDataWriter, HeartbeatWhileDestruction)
}
}

/**
* This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
* It checks that a DataWriter be created with default Qos and a large history depth.
*/
TEST(DDSDataWriter, default_qos_large_history_depth)
{
PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
writer.history_depth(1000).init();
ASSERT_TRUE(writer.isInitialized());
}

#ifdef INSTANTIATE_TEST_SUITE_P
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
#else
Expand Down
80 changes: 77 additions & 3 deletions test/unittest/dds/publisher/DataWriterTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <chrono>
#include <condition_variable>
#include <cstdint>
#include <memory>
#include <mutex>
#include <thread>

Expand All @@ -24,6 +27,7 @@
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/DomainParticipantListener.hpp>
#include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
#include <fastdds/dds/log/Log.hpp>
#include <fastdds/dds/publisher/DataWriter.hpp>
#include <fastdds/dds/publisher/DataWriterListener.hpp>
#include <fastdds/dds/publisher/Publisher.hpp>
Expand All @@ -36,7 +40,6 @@
#include <fastdds/rtps/writer/StatefulWriter.h>

#include <fastdds/publisher/DataWriterImpl.hpp>

#include "../../logging/mock/MockConsumer.h"

namespace eprosima {
Expand Down Expand Up @@ -671,8 +674,10 @@ TEST(DataWriterTests, InvalidQos)
EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 0 is inconsistent
qos.history().depth = 2;
EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos)); // KEEP LAST 2 is OK
qos.resource_limits().max_samples_per_instance = 1;
EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
// KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
// This test will fail whenever we enforce the consistency between depth and max_samples_per_instance.
qos.history().depth = 2000;
EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos));

ASSERT_TRUE(publisher->delete_datawriter(datawriter) == ReturnCode_t::RETCODE_OK);
ASSERT_TRUE(participant->delete_topic(topic) == ReturnCode_t::RETCODE_OK);
Expand Down Expand Up @@ -1518,6 +1523,75 @@ TEST_F(DataWriterUnsupportedTests, UnsupportedDataWriterMethods)
ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
}

TEST(DataWriterTests, history_depth_max_samples_per_instance_warning)
{

/* Setup log so it may catch the expected warning */
Log::ClearConsumers();
MockConsumer* mockConsumer = new MockConsumer("RTPS_QOS_CHECK");
Log::RegisterConsumer(std::unique_ptr<LogConsumer>(mockConsumer));
Log::SetVerbosity(Log::Warning);

/* Create a participant, topic, and a publisher */
DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
PARTICIPANT_QOS_DEFAULT);
ASSERT_NE(participant, nullptr);

TypeSupport type(new TopicDataTypeMock());
type.register_type(participant);

Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
ASSERT_NE(topic, nullptr);

Publisher* publisher = participant->create_publisher(PUBLISHER_QOS_DEFAULT);
ASSERT_NE(publisher, nullptr);

/* Create a datawriter with the QoS that should generate a warning */
DataWriterQos qos;
qos.history().depth = 10;
qos.resource_limits().max_samples_per_instance = 5;
DataWriter* datawriter_1 = publisher->create_datawriter(topic, qos);
ASSERT_NE(datawriter_1, nullptr);

/* Check that the config generated a warning */
auto wait_for_log_entries =
[&mockConsumer](const uint32_t amount, const uint32_t retries, const uint32_t wait_ms) -> size_t
{
size_t entries = 0;
for (uint32_t i = 0; i < retries; i++)
{
entries = mockConsumer->ConsumedEntries().size();
if (entries >= amount)
{
break;
}
std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
}
return entries;
};

const size_t expected_entries = 1;
const uint32_t retries = 4;
const uint32_t wait_ms = 25;
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);

/* Check that the datawriter can send data */
FooType data;
ASSERT_EQ(ReturnCode_t::RETCODE_OK, datawriter_1->write(&data, HANDLE_NIL));

/* Check that a correctly initialized writer does not produce any warning */
qos.history().depth = 10;
qos.resource_limits().max_samples_per_instance = 10;
DataWriter* datawriter_2 = publisher->create_datawriter(topic, qos);
ASSERT_NE(datawriter_2, nullptr);
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);

/* Tear down */
participant->delete_contained_entities();
DomainParticipantFactory::get_instance()->delete_participant(participant);
Log::KillThread();
}

} // namespace dds
} // namespace fastdds
} // namespace eprosima
Expand Down
105 changes: 88 additions & 17 deletions test/unittest/dds/subscriber/DataReaderTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,23 @@
// limitations under the License.

#include <cassert>
#include <chrono>
#include <cstdint>
#include <forward_list>
#include <iostream>
#include <memory>
#include <sstream>
#include <thread>

#include <asio.hpp>

#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include <fastcdr/Cdr.h>

#include <fastdds/dds/builtin/topic/PublicationBuiltinTopicData.hpp>

#include <fastdds/dds/core/condition/WaitSet.hpp>
#include <fastdds/dds/core/Entity.hpp>
#include <fastdds/dds/core/LoanableArray.hpp>
#include <fastdds/dds/core/LoanableCollection.hpp>
Expand All @@ -30,37 +38,31 @@
#include <fastdds/dds/core/status/BaseStatus.hpp>
#include <fastdds/dds/core/status/SampleRejectedStatus.hpp>
#include <fastdds/dds/core/status/SubscriptionMatchedStatus.hpp>

#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/DomainParticipant.hpp>
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
#include <fastdds/dds/domain/DomainParticipantListener.hpp>

#include <fastdds/dds/log/Log.hpp>
#include <fastdds/dds/publisher/DataWriter.hpp>
#include <fastdds/dds/publisher/Publisher.hpp>
#include <fastdds/dds/publisher/qos/DataWriterQos.hpp>
#include <fastdds/dds/publisher/qos/PublisherQos.hpp>

#include <fastdds/dds/subscriber/DataReader.hpp>
#include <fastdds/dds/subscriber/DataReaderListener.hpp>
#include <fastdds/dds/subscriber/Subscriber.hpp>
#include <fastdds/dds/subscriber/SampleInfo.hpp>
#include <fastdds/dds/subscriber/qos/DataReaderQos.hpp>
#include <fastdds/dds/subscriber/qos/SubscriberQos.hpp>

#include <fastdds/dds/subscriber/SampleInfo.hpp>
#include <fastdds/dds/subscriber/Subscriber.hpp>
#include <fastdds/rtps/common/Locator.h>
#include <fastdds/rtps/transport/test_UDPv4TransportDescriptor.h>
#include <fastrtps/utils/IPLocator.h>
#include <fastrtps/xmlparser/XMLProfileManager.h>

#include "../../logging/mock/MockConsumer.h"
#include "FooBoundedType.hpp"
#include "FooBoundedTypeSupport.hpp"

#include "FooType.hpp"
#include "FooTypeSupport.hpp"

#include "../../logging/mock/MockConsumer.h"

#include <fastdds/rtps/transport/test_UDPv4TransportDescriptor.h>
#include <fastrtps/xmlparser/XMLProfileManager.h>

namespace eprosima {
namespace fastdds {
namespace dds {
Expand Down Expand Up @@ -683,9 +685,13 @@ TEST_F(DataReaderTests, InvalidQos)
qos.history().kind = KEEP_LAST_HISTORY_QOS;
qos.history().depth = 0;
EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 0 is inconsistent
qos.history().depth = 2;
qos.resource_limits().max_samples_per_instance = 1;
EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
// KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
// In the reader, this returns RETCODE_INMUTABLE_POLICY, because the depth cannot be changed on run time.
// Because of the implementation, we know de consistency is checked before the inmutability, so by checking the
// return against RETCODE_INMUTABLE_POLICY we are testing that the setting are not considered inconsistent yet.
// This test will fail whenever we enforce the consistency between depth and max_samples_per_instance.
qos.history().depth = 2000;
EXPECT_EQ(ReturnCode_t::RETCODE_IMMUTABLE_POLICY, data_reader_->set_qos(qos));

/* Inmutable QoS */
const ReturnCode_t inmutable_code = ReturnCode_t::RETCODE_IMMUTABLE_POLICY;
Expand Down Expand Up @@ -2659,6 +2665,71 @@ TEST_F(DataReaderTests, delete_contained_entities)
ASSERT_EQ(data_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK);
}

TEST_F(DataReaderTests, history_depth_max_samples_per_instance_warning)
{

/* Setup log so it may catch the expected warning */
Log::ClearConsumers();
MockConsumer* mockConsumer = new MockConsumer("RTPS_QOS_CHECK");
Log::RegisterConsumer(std::unique_ptr<LogConsumer>(mockConsumer));
Log::SetVerbosity(Log::Warning);

/* Create a participant, topic, and a subscriber */
DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
PARTICIPANT_QOS_DEFAULT);
ASSERT_NE(participant, nullptr);

TypeSupport type(new FooTypeSupport());
type.register_type(participant);

Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
ASSERT_NE(topic, nullptr);

Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
ASSERT_NE(subscriber, nullptr);

/* Create a datareader with the QoS that should generate a warning */
DataReaderQos qos;
qos.history().depth = 10;
qos.resource_limits().max_samples_per_instance = 5;
DataReader* datareader_1 = subscriber->create_datareader(topic, qos);
ASSERT_NE(datareader_1, nullptr);

/* Check that the config generated a warning */
auto wait_for_log_entries =
[&mockConsumer](const uint32_t amount, const uint32_t retries, const uint32_t wait_ms) -> size_t
{
size_t entries = 0;
for (uint32_t i = 0; i < retries; i++)
{
entries = mockConsumer->ConsumedEntries().size();
if (entries >= amount)
{
break;
}
std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
}
return entries;
};

const size_t expected_entries = 1;
const uint32_t retries = 4;
const uint32_t wait_ms = 25;
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);

/* Check that a correctly initialized datareader does not produce any warning */
qos.history().depth = 10;
qos.resource_limits().max_samples_per_instance = 10;
DataReader* datareader_2 = subscriber->create_datareader(topic, qos);
ASSERT_NE(datareader_2, nullptr);
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);

/* Tear down */
participant->delete_contained_entities();
DomainParticipantFactory::get_instance()->delete_participant(participant);
Log::KillThread();
}

} // namespace dds
} // namespace fastdds
} // namespace eprosima
Expand Down

0 comments on commit c29020b

Please sign in to comment.