Project

General

Profile

Bug #4538

Congestion mark related elements missing in ControlParameters

Added by Junxiao Shi about 3 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Immediate
Assignee:
Category:
Management
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
1.00 h

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.


Related issues

Related to NFD - Feature #4318: Content Store flush/erase commandClosedJunxiao Shi

Actions
Blocks NFD - Task #4539: Management: collect all TLV-TYPE number assignments on the same pageNew

Actions
#1

Updated by Junxiao Shi about 3 years ago

  • Related to Feature #4318: Content Store flush/erase command added
#2

Updated by Junxiao Shi about 3 years ago

  • Assignee set to Eric Newberry
  • Estimated time set to 2.00 h
#3

Updated by Junxiao Shi about 3 years ago

  • Priority changed from Normal to Immediate
#4

Updated by Davide Pesavento about 3 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.

#5

Updated by Junxiao Shi about 3 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.

#6

Updated by Junxiao Shi about 3 years ago

  • Blocks Task #4539: Management: collect all TLV-TYPE number assignments on the same page added
#7

Updated by Eric Newberry about 3 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.

#8

Updated by Eric Newberry about 3 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.

#9

Updated by Eric Newberry about 3 years ago

Changes to the NCsEntries TLV-TYPE on the wiki have been reverted and the ndn-cxx change has been abandoned.

#10

Updated by Davide Pesavento about 3 years ago

  • Status changed from Code review to Resolved

Eric Newberry wrote:

The TLV-TYPE for NCsEntries has 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 BaseCongestionMarkingInterval and DefaultCongestionThreshold to the TLV-TYPE list on the ControlCommand page.

Ok, I think this issue is resolved then.

#11

Updated by Junxiao Shi about 3 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.

#12

Updated by Davide Pesavento almost 3 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF