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

Expose seqnum via sampleinfo #1484

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

TheFixer
Copy link
Contributor

@TheFixer TheFixer commented Nov 18, 2022

I am building an application using CycloneDDS that requires (read only) access to the sequence numbers of samples that have been published. Because CycloneDDS currently does not provide such access, I experimented myself a bit.

This pull request adds an extra field to the dds_sample_info_t struct that exposes the sequence number of a sample. I am pretty sure there are different ways to do it, but this is at at least one way.

In case you agree that exposing sequence numbers of samples is a good idea, then this pull request might be something to consider. In that case let me know if you agree or disagree with the solution proposed in this pull request. I happy to rework the solution, just let me now.

@eboasson
Copy link
Contributor

Thanks @TheFixer for providing an implementation (I think a partial one, there are places where serdatas get constructed that would need updating, but let's not worry about that too much just yet) of this contentious feature. Always much better to debate concrete proposals.

Assuming I would agree with adding it to the "sample info", and leaving aside the existence of some other places that need analogous changes, there is only one thing I think is "off" in this PR, and that's the name of the field. All the fields in the sample info have these terribly long names, and a simple "seq_no" doesn't really fit in. I'm sure that the spec would have spelled it out as "sequence_number" if it had included it, and that is what would have made its way into the sample info in Cyclone, too.

However, I am not keen on adding it. The sequence number is in my view something internal to the DDS implementation and not metadata that is for the application's consumption (however handy it might come in sometimes), because I am of the opinion that any data that matters to the application should be modelled in the topics. Unfortunately, I'm stuck with all the mostly-useless metadata in sample info that the spec defines ... if it were up to me, very little of that would exist.

That I am against adding it to "sample info" doesn't mean I am against adding it to serdata (indeed, we really do need it there for a variety of reasons: what you are doing, but also in case of shared memory + multi-pathing). Given that there is a function to get hold of serdatas (e.g., dds_readcdr) I think that would be sufficient, but I am happy to hear other opinions. I might even change my position on this!

@hansvanthag
Copy link
Member

Guess that what it boils down to is 'how normal' of an application, a durability-service is/can be.
If its sufficiently 'special' it could even be allowed to exploit some internal API's/mechanisms to perform it's job (such as having access to a sequence-number that 'normal' app's wouldn't have access to).
And i.m.o. a durability-service would be sufficiently 'special' (likely also w.r.t. how to 'play' in a secure environment)

@TheFixer
Copy link
Contributor Author

TheFixer commented Dec 21, 2022

In an attempt to process the review comments for this pull request, the following changes have been made:

  1. The seq_no is removed from dds_sampleinfo_t, so the sequence number is not exposed any more as part of the sampleinfo in the dds api
  2. The sequence number is now part of the serdata struct, and is named 'sequence_number' instead of 'seq_no'.
  3. In ddsi_serdata_init() the sequence number is initialized at 0

Can you let me know if these changes address the review comments, or whether changes are required.

@TheFixer
Copy link
Contributor Author

The sequencenumber (formally called seqnum) has been removed from dds_sampleinfo_t, but is available via the serdata interface. The following (detailed) modifications were made:

  • struct ddsi_serdata_ops is extended with a field called .get_sequencenumber, which implements a functionto retrieve the sequence number from a sample
  • The function .get_sequencenumber is implemented for dds_serdata_ops_cdr, dds_serdata_ops_xcdr2, dds_serdata_ops_cdr_nokey, dds_serdata_ops_xcdr2_nokey. For the builtin_topics, the .get_sequencenumber = 0 (not available)
  • sequence number is initialized to 0
  • For ddsi_serdata_ops_plist and ddsi_serdata_ops_pserop the function .get_sequencenumber is set to 0 (i.e., not available)

TheFixer and others added 21 commits March 19, 2024 10:25
…at the sequence number can be retrieved from serdata

Signed-off-by: TheFixer <[email protected]>
…to the rhc of a durable reader

Signed-off-by: TheFixer <[email protected]>
… first participant is created

Signed-off-by: TheFixer <[email protected]>
… is set to 0 (even though we might want to forbid it as an invalid configuration).

Signed-off-by: TheFixer <[email protected]>
…termine if a quorum is reached

Signed-off-by: TheFixer <[email protected]>
… dispose requests after a timeout expires

Signed-off-by: TheFixer <[email protected]>
mvandenhoek and others added 12 commits March 19, 2024 18:13
@TheFixer TheFixer force-pushed the expose-seqnum-via-sampleinfo branch from a737899 to 4e46317 Compare March 22, 2024 13:06
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

4 participants