Skip to content

Conversation

@EduPonz
Copy link

@EduPonz EduPonz commented Mar 6, 2020

This PR tackles the #1042 issue.

With it, as part of starting a ReaderProxy, a call to acked_changes_set(SequenceNumber_t()) is performed to simulate an initial acknack from the reader. This has the effect of setting the ReaderProxy's low mark to the writer's first available change, which avoids the writer need to iterate through all the changes 0 to low mark the first time it sends data to the reader.

This is OK to do, since an initial heartbeat is sent to the reader notifying the writer's state right after discovery, which already tells the reader that the first change is low mark, so there is no need of sending any gap, nor requesting changes older than that.

@EduPonz EduPonz requested a review from IkerLuengo March 6, 2020 08:46
@EduPonz EduPonz changed the title Refs #7833: Simulate initial acknack on ReaderProxy::start() Simulate initial acknack on ReaderProxy::start() [7838] Mar 6, 2020
Copy link
Contributor

@IkerLuengo IkerLuengo left a comment

Choose a reason for hiding this comment

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

Change the comment on src/cpp/rtps/writer/ReaderProxy.cpp (lines 292-293) to reflect that the special case is now used on late joiners too.

@EduPonz
Copy link
Author

EduPonz commented Mar 6, 2020

Change the comment on src/cpp/rtps/writer/ReaderProxy.cpp (lines 292-293) to reflect that the special case is now used on late joiners too.

Comment updated in
49b6ed1

Copy link
Contributor

@IkerLuengo IkerLuengo left a comment

Choose a reason for hiding this comment

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

LGTM

@richiware
Copy link
Member

richiware commented Mar 6, 2020

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status
    • Unrelated failure

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.

5 participants