Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RtpsUdpInst does not use ConfigStore #4162

Merged
merged 1 commit into from Aug 24, 2023

Conversation

jrw972
Copy link
Contributor

@jrw972 jrw972 commented Jun 8, 2023

Problem

RtpsUdpInst does not use ConfigStore. See #4134.

Solution

Convert RtpsUdpInst to use ConfigStore.

  • Member access has been replaced with getters and setters.

  • Dynamic configuration related to the RtpsRelay is handled via a listener.

  • The RtpsUdpCore caches dynamic configuration values and values that are used frequently.

Notes

This fixes a bug in ConfigStoreImpl where the last valid value was returned instead of the provided default in the event of an error.

@jrw972 jrw972 self-assigned this Jun 8, 2023
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch from adcb957 to 46f9254 Compare June 9, 2023 15:01
@jrw972 jrw972 marked this pull request as draft June 9, 2023 15:01
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch 10 times, most recently from 9aa8321 to 77adcef Compare June 15, 2023 17:03
@jrw972 jrw972 marked this pull request as ready for review June 15, 2023 21:28
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch from 77adcef to 56c2390 Compare June 15, 2023 21:43
@jrw972
Copy link
Contributor Author

jrw972 commented Jun 21, 2023

Safety profile issues caused by DOCGroup/ACE_TAO#2080

@jrw972 jrw972 force-pushed the transport-inst-refactoring branch from 56c2390 to 7d58c1f Compare June 29, 2023 15:09
dds/DCPS/ConfigStoreImpl.cpp Outdated Show resolved Hide resolved
dds/DCPS/transport/rtps_udp/RtpsUdpTransport.cpp Outdated Show resolved Hide resolved
dds/DCPS/transport/rtps_udp/RtpsUdpTransport.cpp Outdated Show resolved Hide resolved
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch 3 times, most recently from 519fcfb to 42054e5 Compare July 7, 2023 21:55
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch 5 times, most recently from aa07818 to 4658d47 Compare July 28, 2023 13:53
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch from 4658d47 to 1037da1 Compare July 28, 2023 18:37
@jrw972 jrw972 force-pushed the transport-inst-refactoring branch 8 times, most recently from ba9c1da to f08732e Compare August 8, 2023 17:57
dds/DCPS/RTPS/Sedp.cpp Outdated Show resolved Hide resolved
dds/DCPS/RTPS/Sedp.cpp Outdated Show resolved Hide resolved
return to_dds_string(duration.sec) + '.' + left_pad(to_dds_string(duration.nanosec), '0', 9);
}

ACE_INLINE OpenDDS_Dcps_Export
bool from_dds_string(const String& str, DDS::Duration_t& duration)
{
if (str == "DDS::INFINITE_DURATION") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but this isn't a constant that exists anywhere as far as I know, so does it have to be phrased like this? Even if it was, could it just be something shorter like infinity, inf, or forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSON spec puts forth DURATION_INFINITY. Do you think that's an acceptable option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's alright.

dds/DCPS/transport/rtps_udp/RtpsUdpInst.cpp Outdated Show resolved Hide resolved
docs/news.d/config_store.rst Show resolved Hide resolved

store1.set("PREFIX1_key", "value");
EXPECT_TRUE(take_has_prefix(reader, "PREFIX1"));
EXPECT_FALSE(take_has_prefix(reader, "PREFIX1"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really related to this code, but reading it made me think of it. Does the config store warn if a value wasn't used? If not, would it be possible? There's different things grabbing things from the key store, so I assume that could be tracked as bool on ConfigPair and at some point you'd know if something was used or not. If someone misunderstood how to make a key value or mistyped one, which could happen to a user or one of us, then it seems like it would be good for some sort of feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. The InternalDataReader keeps the view state and sample state. So, if everything accesses the config through the same reader, then those states would reflect if a value was accessed or not. Writing the function to do the scan would be easy. The challenge would be when to invoke it.

@jrw972 jrw972 force-pushed the transport-inst-refactoring branch 2 times, most recently from 3e9f3ca to 58b8253 Compare August 10, 2023 20:39
docs/news.d/config_store.rst Outdated Show resolved Hide resolved
Problem
-------

`RtpsUdpInst` does not use `ConfigStore`.  See OpenDDS#4134.

Solution
--------

Convert `RtpsUdpInst` to use `ConfigStore`.

* Member access has been replaced with getters and setters.

* Dynamic configuration related to the RtpsRelay is handled via a
  listener.

* The `RtpsUdpCore` caches dynamic configuration values and values
  that are used frequently.

Notes
-----

This fixes a bug in `ConfigStoreImpl` where the last valid value was
returned instead of the provided default in the event of an error.
@jrw972 jrw972 merged commit 3e29641 into OpenDDS:master Aug 24, 2023
118 of 124 checks passed
@jrw972 jrw972 deleted the transport-inst-refactoring branch September 7, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants