Skip to content

Commit 12201f4

Browse files
MiguelCompanyEduPonz
MiguelCompany
and
EduPonz
authored
Just show warning when inconsistency between depth and max_samples_per_instance (#4417)
* Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]>
1 parent 6cb5ebb commit 12201f4

File tree

6 files changed

+196
-34
lines changed

6 files changed

+196
-34
lines changed

‎src/cpp/fastdds/publisher/DataWriterImpl.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1882,9 +1882,11 @@ ReturnCode_t DataWriterImpl::check_qos(
18821882
qos.resource_limits().max_samples_per_instance > 0 &&
18831883
qos.history().depth > qos.resource_limits().max_samples_per_instance)
18841884
{
1885-
EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK,
1886-
"HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
1887-
return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
1885+
EPROSIMA_LOG_WARNING(RTPS_QOS_CHECK,
1886+
"HISTORY DEPTH '" << qos.history().depth <<
1887+
"' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
1888+
"'. Consistency rule: depth <= max_samples_per_instance." <<
1889+
" Effectively using max_samples_per_instance as depth.");
18881890
}
18891891
return ReturnCode_t::RETCODE_OK;
18901892
}

‎src/cpp/fastdds/subscriber/DataReaderImpl.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1510,9 +1510,11 @@ ReturnCode_t DataReaderImpl::check_qos(
15101510
qos.resource_limits().max_samples_per_instance > 0 &&
15111511
qos.history().depth > qos.resource_limits().max_samples_per_instance)
15121512
{
1513-
EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK,
1514-
"HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
1515-
return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
1513+
EPROSIMA_LOG_WARNING(RTPS_QOS_CHECK,
1514+
"HISTORY DEPTH '" << qos.history().depth <<
1515+
"' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
1516+
"'. Consistency rule: depth <= max_samples_per_instance." <<
1517+
" Effectively using max_samples_per_instance as depth.");
15161518
}
15171519
return ReturnCode_t::RETCODE_OK;
15181520
}

‎test/blackbox/common/DDSBlackboxTestsDataReader.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,17 @@ TEST(DDSDataReader, ConsistentReliabilityWhenIntraprocess)
297297
xmlparser::XMLProfileManager::library_settings(library_settings);
298298
}
299299

300+
/**
301+
* This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
302+
* It checks that a DataReader be created with default Qos and a large history depth.
303+
*/
304+
TEST(DDSDataReader, default_qos_large_history_depth)
305+
{
306+
PubSubReader<HelloWorldPubSubType> reader(TEST_TOPIC_NAME);
307+
reader.history_depth(1000).init();
308+
ASSERT_TRUE(reader.isInitialized());
309+
}
310+
300311
#ifdef INSTANTIATE_TEST_SUITE_P
301312
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
302313
#else

‎test/blackbox/common/DDSBlackboxTestsDataWriter.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,17 @@ TEST(DDSDataWriter, HeartbeatWhileDestruction)
421421
}
422422
}
423423

424+
/**
425+
* This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
426+
* It checks that a DataWriter be created with default Qos and a large history depth.
427+
*/
428+
TEST(DDSDataWriter, default_qos_large_history_depth)
429+
{
430+
PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
431+
writer.history_depth(1000).init();
432+
ASSERT_TRUE(writer.isInitialized());
433+
}
434+
424435
#ifdef INSTANTIATE_TEST_SUITE_P
425436
#define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
426437
#else

‎test/unittest/dds/publisher/DataWriterTests.cpp

Lines changed: 78 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
#include <chrono>
1516
#include <condition_variable>
17+
#include <cstdint>
18+
#include <memory>
1619
#include <mutex>
1720
#include <thread>
1821

@@ -28,6 +31,7 @@
2831
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
2932
#include <fastdds/dds/domain/DomainParticipantListener.hpp>
3033
#include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
34+
#include <fastdds/dds/log/Log.hpp>
3135
#include <fastdds/dds/publisher/DataWriter.hpp>
3236
#include <fastdds/dds/publisher/DataWriterListener.hpp>
3337
#include <fastdds/dds/publisher/Publisher.hpp>
@@ -40,9 +44,8 @@
4044
#include <fastdds/rtps/writer/RTPSWriter.h>
4145
#include <fastdds/rtps/writer/StatefulWriter.h>
4246

43-
#include "../../logging/mock/MockConsumer.h"
44-
4547
#include "../../common/CustomPayloadPool.hpp"
48+
#include "../../logging/mock/MockConsumer.h"
4649

4750
namespace eprosima {
4851
namespace fastdds {
@@ -725,8 +728,10 @@ TEST(DataWriterTests, InvalidQos)
725728
EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 0 is inconsistent
726729
qos.history().depth = 2;
727730
EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos)); // KEEP LAST 2 is OK
728-
qos.resource_limits().max_samples_per_instance = 1;
729-
EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
731+
// KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
732+
// This test will fail whenever we enforce the consistency between depth and max_samples_per_instance.
733+
qos.history().depth = 2000;
734+
EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos));
730735

731736
ASSERT_TRUE(publisher->delete_datawriter(datawriter) == ReturnCode_t::RETCODE_OK);
732737
ASSERT_TRUE(participant->delete_topic(topic) == ReturnCode_t::RETCODE_OK);
@@ -2066,6 +2071,75 @@ TEST(DataWriterTests, CustomPoolCreation)
20662071
DomainParticipantFactory::get_instance()->delete_participant(participant);
20672072
}
20682073

2074+
TEST(DataWriterTests, history_depth_max_samples_per_instance_warning)
2075+
{
2076+
2077+
/* Setup log so it may catch the expected warning */
2078+
Log::ClearConsumers();
2079+
MockConsumer* mockConsumer = new MockConsumer("RTPS_QOS_CHECK");
2080+
Log::RegisterConsumer(std::unique_ptr<LogConsumer>(mockConsumer));
2081+
Log::SetVerbosity(Log::Warning);
2082+
2083+
/* Create a participant, topic, and a publisher */
2084+
DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
2085+
PARTICIPANT_QOS_DEFAULT);
2086+
ASSERT_NE(participant, nullptr);
2087+
2088+
TypeSupport type(new TopicDataTypeMock());
2089+
type.register_type(participant);
2090+
2091+
Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
2092+
ASSERT_NE(topic, nullptr);
2093+
2094+
Publisher* publisher = participant->create_publisher(PUBLISHER_QOS_DEFAULT);
2095+
ASSERT_NE(publisher, nullptr);
2096+
2097+
/* Create a datawriter with the QoS that should generate a warning */
2098+
DataWriterQos qos;
2099+
qos.history().depth = 10;
2100+
qos.resource_limits().max_samples_per_instance = 5;
2101+
DataWriter* datawriter_1 = publisher->create_datawriter(topic, qos);
2102+
ASSERT_NE(datawriter_1, nullptr);
2103+
2104+
/* Check that the config generated a warning */
2105+
auto wait_for_log_entries =
2106+
[&mockConsumer](const uint32_t amount, const uint32_t retries, const uint32_t wait_ms) -> size_t
2107+
{
2108+
size_t entries = 0;
2109+
for (uint32_t i = 0; i < retries; i++)
2110+
{
2111+
entries = mockConsumer->ConsumedEntries().size();
2112+
if (entries >= amount)
2113+
{
2114+
break;
2115+
}
2116+
std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
2117+
}
2118+
return entries;
2119+
};
2120+
2121+
const size_t expected_entries = 1;
2122+
const uint32_t retries = 4;
2123+
const uint32_t wait_ms = 25;
2124+
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
2125+
2126+
/* Check that the datawriter can send data */
2127+
FooType data;
2128+
ASSERT_EQ(ReturnCode_t::RETCODE_OK, datawriter_1->write(&data, HANDLE_NIL));
2129+
2130+
/* Check that a correctly initialized writer does not produce any warning */
2131+
qos.history().depth = 10;
2132+
qos.resource_limits().max_samples_per_instance = 10;
2133+
DataWriter* datawriter_2 = publisher->create_datawriter(topic, qos);
2134+
ASSERT_NE(datawriter_2, nullptr);
2135+
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
2136+
2137+
/* Tear down */
2138+
participant->delete_contained_entities();
2139+
DomainParticipantFactory::get_instance()->delete_participant(participant);
2140+
Log::KillThread();
2141+
}
2142+
20692143
} // namespace dds
20702144
} // namespace fastdds
20712145
} // namespace eprosima

‎test/unittest/dds/subscriber/DataReaderTests.cpp

Lines changed: 86 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,65 +15,58 @@
1515
#include <array>
1616
#include <cassert>
1717
#include <chrono>
18+
#include <cstdint>
1819
#include <forward_list>
1920
#include <iostream>
2021
#include <memory>
2122
#include <sstream>
2223
#include <thread>
2324
#include <type_traits>
2425

26+
#include <asio.hpp>
27+
2528
#include <gmock/gmock.h>
2629
#include <gtest/gtest.h>
2730

2831
#include <fastcdr/Cdr.h>
2932

3033
#include <fastdds/dds/builtin/topic/PublicationBuiltinTopicData.hpp>
31-
34+
#include <fastdds/dds/core/condition/WaitSet.hpp>
3235
#include <fastdds/dds/core/Entity.hpp>
3336
#include <fastdds/dds/core/LoanableArray.hpp>
3437
#include <fastdds/dds/core/LoanableCollection.hpp>
3538
#include <fastdds/dds/core/LoanableSequence.hpp>
3639
#include <fastdds/dds/core/StackAllocatedSequence.hpp>
37-
#include <fastdds/dds/core/condition/WaitSet.hpp>
3840
#include <fastdds/dds/core/status/BaseStatus.hpp>
3941
#include <fastdds/dds/core/status/SampleRejectedStatus.hpp>
4042
#include <fastdds/dds/core/status/SubscriptionMatchedStatus.hpp>
41-
42-
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
4343
#include <fastdds/dds/domain/DomainParticipant.hpp>
44+
#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
4445
#include <fastdds/dds/domain/DomainParticipantListener.hpp>
45-
46+
#include <fastdds/dds/log/Log.hpp>
4647
#include <fastdds/dds/publisher/DataWriter.hpp>
4748
#include <fastdds/dds/publisher/Publisher.hpp>
4849
#include <fastdds/dds/publisher/qos/DataWriterQos.hpp>
4950
#include <fastdds/dds/publisher/qos/PublisherQos.hpp>
50-
5151
#include <fastdds/dds/subscriber/DataReader.hpp>
5252
#include <fastdds/dds/subscriber/DataReaderListener.hpp>
53-
#include <fastdds/dds/subscriber/SampleInfo.hpp>
54-
#include <fastdds/dds/subscriber/Subscriber.hpp>
5553
#include <fastdds/dds/subscriber/qos/DataReaderQos.hpp>
5654
#include <fastdds/dds/subscriber/qos/SubscriberQos.hpp>
57-
55+
#include <fastdds/dds/subscriber/SampleInfo.hpp>
56+
#include <fastdds/dds/subscriber/Subscriber.hpp>
5857
#include <fastdds/rtps/common/Locator.h>
59-
#include <fastrtps/utils/IPLocator.h>
60-
61-
#include "FooBoundedType.hpp"
62-
#include "FooBoundedTypeSupport.hpp"
63-
64-
#include "FooType.hpp"
65-
#include "FooTypeSupport.hpp"
66-
67-
#include "../../logging/mock/MockConsumer.h"
68-
6958
#include <fastdds/rtps/transport/test_UDPv4TransportDescriptor.h>
59+
#include <fastrtps/utils/IPLocator.h>
7060
#include <fastrtps/xmlparser/XMLProfileManager.h>
7161

7262
#include "../../common/CustomPayloadPool.hpp"
63+
#include "../../logging/mock/MockConsumer.h"
7364
#include "fastdds/dds/common/InstanceHandle.hpp"
7465
#include "fastdds/dds/core/policy/QosPolicies.hpp"
75-
76-
#include <asio.hpp>
66+
#include "FooBoundedType.hpp"
67+
#include "FooBoundedTypeSupport.hpp"
68+
#include "FooType.hpp"
69+
#include "FooTypeSupport.hpp"
7770

7871
#if defined(__cplusplus_winrt)
7972
#define GET_PID GetCurrentProcessId
@@ -711,9 +704,13 @@ TEST_F(DataReaderTests, InvalidQos)
711704
qos.history().kind = KEEP_LAST_HISTORY_QOS;
712705
qos.history().depth = 0;
713706
EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 0 is inconsistent
714-
qos.history().depth = 2;
715-
qos.resource_limits().max_samples_per_instance = 1;
716-
EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
707+
// KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
708+
// In the reader, this returns RETCODE_INMUTABLE_POLICY, because the depth cannot be changed on run time.
709+
// Because of the implementation, we know de consistency is checked before the inmutability, so by checking the
710+
// return against RETCODE_INMUTABLE_POLICY we are testing that the setting are not considered inconsistent yet.
711+
// This test will fail whenever we enforce the consistency between depth and max_samples_per_instance.
712+
qos.history().depth = 2000;
713+
EXPECT_EQ(ReturnCode_t::RETCODE_IMMUTABLE_POLICY, data_reader_->set_qos(qos));
717714

718715
/* Inmutable QoS */
719716
const ReturnCode_t inmutable_code = ReturnCode_t::RETCODE_IMMUTABLE_POLICY;
@@ -3655,6 +3652,71 @@ TEST_F(DataReaderTests, UpdateInmutableQos)
36553652
DomainParticipantFactory::get_instance()->delete_participant(participant);
36563653
}
36573654

3655+
TEST_F(DataReaderTests, history_depth_max_samples_per_instance_warning)
3656+
{
3657+
3658+
/* Setup log so it may catch the expected warning */
3659+
Log::ClearConsumers();
3660+
MockConsumer* mockConsumer = new MockConsumer("RTPS_QOS_CHECK");
3661+
Log::RegisterConsumer(std::unique_ptr<LogConsumer>(mockConsumer));
3662+
Log::SetVerbosity(Log::Warning);
3663+
3664+
/* Create a participant, topic, and a subscriber */
3665+
DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
3666+
PARTICIPANT_QOS_DEFAULT);
3667+
ASSERT_NE(participant, nullptr);
3668+
3669+
TypeSupport type(new FooTypeSupport());
3670+
type.register_type(participant);
3671+
3672+
Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
3673+
ASSERT_NE(topic, nullptr);
3674+
3675+
Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
3676+
ASSERT_NE(subscriber, nullptr);
3677+
3678+
/* Create a datareader with the QoS that should generate a warning */
3679+
DataReaderQos qos;
3680+
qos.history().depth = 10;
3681+
qos.resource_limits().max_samples_per_instance = 5;
3682+
DataReader* datareader_1 = subscriber->create_datareader(topic, qos);
3683+
ASSERT_NE(datareader_1, nullptr);
3684+
3685+
/* Check that the config generated a warning */
3686+
auto wait_for_log_entries =
3687+
[&mockConsumer](const uint32_t amount, const uint32_t retries, const uint32_t wait_ms) -> size_t
3688+
{
3689+
size_t entries = 0;
3690+
for (uint32_t i = 0; i < retries; i++)
3691+
{
3692+
entries = mockConsumer->ConsumedEntries().size();
3693+
if (entries >= amount)
3694+
{
3695+
break;
3696+
}
3697+
std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
3698+
}
3699+
return entries;
3700+
};
3701+
3702+
const size_t expected_entries = 1;
3703+
const uint32_t retries = 4;
3704+
const uint32_t wait_ms = 25;
3705+
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
3706+
3707+
/* Check that a correctly initialized datareader does not produce any warning */
3708+
qos.history().depth = 10;
3709+
qos.resource_limits().max_samples_per_instance = 10;
3710+
DataReader* datareader_2 = subscriber->create_datareader(topic, qos);
3711+
ASSERT_NE(datareader_2, nullptr);
3712+
ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
3713+
3714+
/* Tear down */
3715+
participant->delete_contained_entities();
3716+
DomainParticipantFactory::get_instance()->delete_participant(participant);
3717+
Log::KillThread();
3718+
}
3719+
36583720
int main(
36593721
int argc,
36603722
char** argv)

0 commit comments

Comments
 (0)