Bug #4538
closed
Congestion mark related elements missing in ControlParameters
100%
Description
ndn-cxx:commit:07d05c97b435a8de5ff76986df6a0cb3a50a6484 added several congestion mark related elements to ControlParameters class without adding them to ControlCommand protocol page. In particular, BaseCongestionMarkingInterval has a conflicting TLV-TYPE number with another element that has an assignment on this page, and causes NFD crash (observed in #4318-13).
Please add the congestion marking related elements and their TLV-TYPE assignments to ControlCommand ASAP, to decrease the likelihood of conflicts in the future.
Incidentally, we need a better, more foolproof, way of managing these assignments. But that's a separate issue.
Updated by Junxiao Shi over 6 years ago
- Related to Feature #4318: Content Store flush/erase command added
Updated by Junxiao Shi over 6 years ago
- Assignee set to Eric Newberry
- Estimated time set to 2.00 h
Updated by Junxiao Shi over 6 years ago
- Priority changed from Normal to Immediate
Updated by Davide Pesavento over 6 years ago
- Description updated (diff)
- Estimated time changed from 2.00 h to 0.10 h
Renumbering of BaseCongestionMarkingInterval is not going to happen. We have a full implementation based on it and we already shipped that code (and that TLV assignment) in v0.6.1. It is way simpler to just change the assignment for NCsEntries, since it was just merged and is still unimplemented in NFD.
Updated by Junxiao Shi over 6 years ago
- Estimated time changed from 0.10 h to 1.00 h
The spec has to be updated first. Completing that in 6 minutes is impossible.
Updated by Junxiao Shi over 6 years ago
- Blocks Task #4539: Management: collect all TLV-TYPE number assignments on the same page added
Updated by Eric Newberry over 6 years ago
- Status changed from New to In Progress
- % Done changed from 0 to 30
The TLV-TYPE for NCsEntries has been changed to 137 on the ControlCommand, CsMgmt, and ForwarderStatus wiki pages. I also added BaseCongestionMarkingInterval and DefaultCongestionThreshold to the TLV-TYPE list on the ControlCommand page.
Updated by Eric Newberry over 6 years ago
- Status changed from In Progress to Code review
- % Done changed from 30 to 100
Update to ndn-cxx has been pushed to Gerrit.
Updated by Eric Newberry over 6 years ago
Changes to the NCsEntries TLV-TYPE on the wiki have been reverted and the ndn-cxx change has been abandoned.
Updated by Davide Pesavento over 6 years ago
- Status changed from Code review to Resolved
Eric Newberry wrote:
The TLV-TYPE for
NCsEntrieshas been changed to 137 on the ControlCommand, CsMgmt, and ForwarderStatus wiki pages.
You were not supposed to do this. Changing CsInfo and FwStatus datasets is just wrong because it changes the protocol in incompatible ways. ControlCommand updates for NCsEntries will be done by Junxiao in #4318 (I guess).
I also added
BaseCongestionMarkingIntervalandDefaultCongestionThresholdto the TLV-TYPE list on the ControlCommand page.
Ok, I think this issue is resolved then.
Updated by Junxiao Shi over 6 years ago
Changing CsInfo and FwStatus datasets is just wrong because it changes the protocol in incompatible ways.
NFD Management never guaranteed backwards compatibility.
ControlCommand updates for NCsEntries will be done by Junxiao in #4318
I won’t make this change, but neither would I object to such change.
Renumbering of BaseCongestionMarkingInterval is not going to happen. We have a full implementation based on it and we already shipped that code (and that TLV assignment) in v0.6.1.
It can happen, given the assignment was not published in protocol spec and thus is internal.
Updated by Davide Pesavento over 6 years ago
- Status changed from Resolved to Closed