Task #4787
closed
Consolidate src/tlv/**lsa** into src/**lsa**
100%
Description
Seems like duplicated functionality. Also then we can use wireEncode for LSAs instead of creating a string manually to represent the LSA.
Updated by Ashlesh Gawande almost 5 years ago
- Status changed from New to Code review
- Assignee changed from Saurab Dulal to Ashlesh Gawande
- % Done changed from 0 to 100
https://gerrit.named-data.net/c/NLSR/+/5935
Major changes:
tlv/lsa classes are removed. wireEncode/wireDecode functions from it are moved to lsa/lsa where they are modified as followed:
- Lsa (Used to be LsaInfo)
- Use ndn::tlv::Name instead of ndn::tlv::nlsr::OriginRouter, OriginRouter is removed from tlvs
- Use expiration timepoint as string instead of expiration period as was the case in LsaInfo
- Clients are expected to subtracted from current time if they want to show time period
- Nlsrc is modified to do so
- Adjacent: Include status as AdjacencyStatus as well as number of interest timed out as AdjacencyInterestTimedOutNo
If these changes are acceptable then I can update the wiki pages. This will be a breaking change.
Serialize/Deserialize functions are removed from Lsa classes and replaced with the newly moved wireEncode/wireDecode.
Lsdb and Dataset publisher are now using these to send LSAs on the wire.
All the other changes are to either facilitate the above or cleaning up the code as seen during the move.
The change deletes ~3000 lines of code including tests (~1000 added, ~4000 deleted).
This improves clean compilation by 2 seconds without tests and 6 seconds with tests on my machine.
Updated by Ashlesh Gawande over 4 years ago
- Is duplicate of Feature #4267: Remove explicit TLV classes added
Updated by Ashlesh Gawande over 4 years ago
Adjacent: Include status as AdjacencyStatus as well as number of interest timed out as AdjacencyInterestTimedOutNo
This is redundant info that NLSR receiving the AdjLsa does not use. So leaning towards not sending it.
I put it there initially to be complete and so that printing them using ostream from AdjLsa in nlsrc prints non-default values. Both arguments seem weak - so not sure if there is any point in including them. Currently NLSR does not send this info and uses the default value when decoding AdjLsa from the wire.
Clients are expected to subtracted from current time if they want to show time period
If subtraction is negative clients need to put some default value.
An option to solve this is to include also Expiration Period in addition to the timestamp in the Lsa. (Some complaints on the mailing list: https://www.lists.cs.ucla.edu/pipermail/ndn-interest/2019-June/002483.html). Also the existing TLV encoding did include Expiration period and not Expiration Point - so this would maintain that part.
Also added some unit tests for testing encoding (in place of existing tlv tests which are removed in this change).
Ref:
- Proposed: https://redmine.named-data.net/projects/nlsr/wiki/Proposed_Common_TLV_for_NLSR-to-NLSR_and_NLSR-to-NLSRC_communication
- Current Nlsr-to-nlsrc: https://redmine.named-data.net/projects/nlsr/wiki/LSDB_DataSet
- (Only difference from Nlsr-to-Nlsr is that Lsa in Nlsr-to-Nlsr contains ExpirationTimePoint instead of ExpirationPeriod)
Updated by Ashlesh Gawande over 4 years ago
Correct mailing list link outlining the problem with using ExpirationTime set at the sender:
https://www.lists.cs.ucla.edu/pipermail/ndn-interest/2019-August/002569.html
Updated by Ashlesh Gawande over 4 years ago
https://redmine.named-data.net/projects/nlsr/wiki/Proposed_Common_TLV_for_NLSR-to-NLSR_and_NLSR-to-NLSRC_communication
is updated to remove ExpirationPeriod from LSA:
The reason to not use ExpirationPeriod even though it would solve this particular clock sync problem in NLSR, other things like certificate validity still depend on loose clock sync b/w nodes. So clock synchronization problem between nodes is independent of NLSR.
Current change https://gerrit.named-data.net/c/NLSR/+/5935 (patchset 10,11) follows the updated proposed specification above.
Updated by Saurab Dulal over 4 years ago
Ashlesh Gawande wrote:
https://redmine.named-data.net/projects/nlsr/wiki/Proposed_Common_TLV_for_NLSR-to-NLSR_and_NLSR-to-NLSRC_communication
is updated to remove ExpirationPeriod from LSA:The reason to not use ExpirationPeriod even though it would solve this particular clock sync problem in NLSR, other things like certificate validity still depend on loose clock sync b/w nodes. So clock synchronization problem between nodes is independent of NLSR.
Personally, I have read about this so many times but still don't have a strong opinion on what can be the right choice. But, the way you have explained and implemented might be the right way to go for now.
Current change https://gerrit.named-data.net/c/NLSR/+/5935 (patchset 10,11) follows the updated proposed specification above.
Thanks for the detailed explanation of the changes.
Updated by Ashlesh Gawande over 4 years ago
- Status changed from Code review to Closed