Project

General

Profile

Task #2841

NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceId

Added by Junxiao Shi over 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
High
Assignee:
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

Design NDNLPv2 packet format API for:

  • network NACK
  • fragmentation
  • NextHopFaceId
  • CachingPolicy
  • IncomingFaceId


Related issues

Blocked by NFD - Task #2763: NDNLPv2: NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceIdClosed

Blocks ndn-cxx - Task #2874: Decide directory structure for NDNLPv2Closed

Blocks ndn-cxx - Feature #2875: NDNLPv2: TLV-TYPE codesClosed

Blocks ndn-cxx - Feature #2876: NDNLPv2: simple value type declarationClosed

Blocks ndn-cxx - Feature #2877: NDNLPv2: NackReason abstractionAbandoned

Blocks ndn-cxx - Feature #2878: NDNLPv2: Nack abstractionClosed

History

#1 Updated by Junxiao Shi over 4 years ago

  • Blocked by Task #2763: NDNLPv2: NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceId added

#2 Updated by Junxiao Shi over 4 years ago

  • Description updated (diff)
  • Status changed from New to In Progress

#3 Updated by Junxiao Shi over 4 years ago

  • % Done changed from 0 to 30

Written: packet format API

Upcoming:

  • NACK struct
  • tagging network layer packet

#4 Updated by Junxiao Shi over 4 years ago

  • % Done changed from 30 to 60

Written: NACK and CachingPolicy

#5 Updated by Junxiao Shi over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 60 to 100

Written: attachment to Interest/Data

Draft design is now complete.

#6 Updated by Eric Newberry over 4 years ago

I'm a bit confused about some of the field declarations relating to Boost Concepts(?). Particularly, I'm confused about the field declarations, like the following (line 98):

typedef /* ValueType=Sequence */ SequenceField;

In particular, what does ValueType mean? I'm guessing it has something to do with Concepts. I've never worked with Boost Concepts before, so foregive me if my question is unclear.

#7 Updated by Eric Newberry over 4 years ago

Also, I noticed that the example() function directly creates NdnlpPackets. Splitting Data and creating NdnlpPackets is directly handled by the forwarding daemon and not this API?

#8 Updated by Junxiao Shi over 4 years ago

Answer to note-6:

ValueType=Sequence is unrelated to Boost.
It refers to an implementation-defined type such that std::is_same<SequenceField::ValueType, Sequence>.

Answer to note-7:

Fragmenting a network layer packet into NdnlpPackets is the responsibility of NDNLPv2 fragmentation feature implementation.
It's not shown at packet format API level.

#9 Updated by Davide Pesavento over 4 years ago

  • Why is this necessary? choose one nesting or the other, having both will only cause confusion IMO.
namespace tlv = ::ndn::tlv::ndnlp2;
  • I prefer the name NegativeAck but no big deal.
class Nack; // declared in network layer API
  • What is the NackReasonConcrete concept supposed to check?

#10 Updated by Junxiao Shi over 4 years ago

Answer to note-9:

::ndn::tlv::ndnlp2 namespace is the official spelling.

::ndn::ndnlp2::tlv is an alias to allow NDNLPv2 API and implementation to write just tlv::Element.

NACK is a concept described in several papers.
It's cumbersome to spell out the full title.

NackReasonConcrete is a subclass of NackReason, and also has the TlvType typedef.

NackReason base class is necessary so that when code wants to store the reason without inspecting it, it can be stored as the base class.

#11 Updated by Davide Pesavento over 4 years ago

Junxiao Shi wrote:

::ndn::tlv::ndnlp2 namespace is the official spelling.

What does official mean? Official regarding to what? Why can't the other one be the official naming since it's more convenient to use?

#12 Updated by Junxiao Shi over 4 years ago

Answer to note-11:

::ndn::tlv::ndnlp2 is official spelling because all other TLV-TYPE codes (eg. codes used by NFD Management protocol in ndn-cxx) are under ::ndn::tlv namespace, not ::ndn::X::tlv.

#13 Updated by Alex Afanasyev over 4 years ago

Several mixed comments about API and spec


  • Why not to call namespace ndn::lp2. ndn::ndnlp2 is a little repetitive.
    Also. Do we really need to emphasize "version 2"? I would just call ndn::lp or ndn::ndnlp

  • Agree with Davide about tlv namespace. Is there a single use of ndn::tlv::ndnlp2? For other cases (really just ndn::tlv::nfd and ndn::tlv::security) we may have made mistake. My original intention was to use namespace to disambiguate uses. Either way (tlv::nfd and nfd::tlv) provide disambiguation. If the latter easier to use (and we probably using it), then there is no reason to repeat the error.

  • API includes too many repetitions. "Ndnlp" prefix should be omitted in many cases, given it is already in the namespace.

  • (for spec only) Can we call "NdnlpHeaderExtension" a "Feature"?

  • (for spec. Network NACK) Why don't we call "Nack" as "NackReason" or just "Reason". The description already calls it this way. For "DuplicataNack" and "GiveUpNack" I would lose "Nack" suffix. Why NACK reason should have "Nack" in it's name?

  • Why NdnlpPacket is assigned code 100 (0x64)?


  • What is field_location_tags::Base ?

  • Don't understand why we have such a distinction between NdnlpSequence and all other features. The behavior is exactly the same.

  • Should "Field" concept be renamed to "Feature" (and relevant typedefs from xxxField to xxxFeature)? Or, this would be preferred for me, completely remove the suffix.

  • We probably should have conditional implementation for (if possible), given it make sense only for features that can be repeated.

    template<typename FIELD>
    std::vector<FIELD::ValueType>
    list();
    
    template<typename FIELD>
    NdnlpPacket&
    add(const FIELD::ValueType& value);
    

  • How are you planning to use FieldSet?

  • Why tlv::Nack is derived from TagHost? How to use Nack?

#14 Updated by Junxiao Shi over 4 years ago

Answers to note-13:

See #2763 note-10 for answers to spec questions.

Why not to call namespace ndn::lp2. ndn::ndnlp2 is a little repetitive.
Also. Do we really need to emphasize "version 2"? I would just call ndn::lp or ndn::ndnlp

lp aka "link protocol" is a generic concept, which includes NDNLP and other link protocols.
NDNLP specifically refers to the protocol we are implementing.

2 is necessary because nfd::ndnlp contains the implementation for NDNLP-TLV, and nfd::ndnlp2 namespace will be needed for certain feature implementations.

Agree with Davide about tlv namespace. Is there a single use of ndn::tlv::ndnlp2? For other cases (really just ndn::tlv::nfd and ndn::tlv::security) we may have made mistake. My original intention was to use namespace to disambiguate uses. Either way (tlv::nfd and nfd::tlv) provide disambiguation. If the latter easier to use (and we probably using it), then there is no reason to repeat the error.

Agreed. I didn't know ndn::tlv::nfd was a mistake.

API includes too many repetitions. "Ndnlp" prefix should be omitted in many cases, given it is already in the namespace.

"Ndnlp" prefix appears at:

  • NdnlpPacket
  • tlv::Ndnlp*

It's unwise to rename NdnlpPacket to Packet, because Packet is too generic.

Constants under tlv namespace should precisely follow the NDNLPv2 spec.

What is field_location_tags::Base ?

This is the base class of the real tags.

It's used to implement the concept check for Field where Field::FieldLocation is asserted to be a subclass of field_location_tags::Base.

Don't understand why we have such a distinction between NdnlpSequence and all other features. The behavior is exactly the same.

NdnlpSequence field is not a header extension.

It must appear first under NdnlpHeader element, before any header extension field.

Should "Field" concept be renamed to "Feature" (and relevant typedefs from xxxField to xxxFeature)? Or, this would be preferred for me, completely remove the suffix.

No. See #2763 note-10: feature and field are distinct concepts.

Although dropping the "Field" suffix makes code look nicer (eg. pkt->get<Sequence>()), there would be implementation difficulties due to name clashing.
For example:

typedef uint64_t Sequence; // value type for NdnlpSequence field
typedef /* implementation-defined */ SequenceField; // field declaration for NdnlpSequence field

Renaming SequenceField to Sequence causes name clashing.

We probably should have conditional implementation for (if possible), given it make sense only for features that can be repeated.

    template<typename FIELD>
    std::vector<FIELD::ValueType>
    list();

    template<typename FIELD>
    NdnlpPacket&
    add(const FIELD::ValueType& value);

This is feasible but unnecessary.

Also note that set is defined to depend on add.

How are you planning to use FieldSet?

NdnlpPacket::wireEncode and NdnlpPacket::wireDecode can iterate over FieldSet, once per distinct FieldLocation, to perform encoding and decoding.

Why tlv::Nack is derived from TagHost? How to use Nack?

tlv::Nack is NOT derived from TagHost.

Nack is derived from TagHost. It's a first-class packet just like Interest and Data.

#15 Updated by Junxiao Shi over 4 years ago

20150608 conference call reviewed revision 59f6bcc13914b77f45fc27024d5330fea4428f64.

Some comments are:

  • The namespace could be just ndn::lp. If someone creates another link protocol, that protocol should find a namespace that doesn't conflict with this.
  • Ndnlp prefix of most types should be dropped.
  • As part of spec change, Sequence becomes a header field and does not need a separate location tag.

#17 Updated by Junxiao Shi over 4 years ago

  • Status changed from Resolved to Closed

20150610 conference call approved the design.

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/3341e7b03cb6d7e7f3f7085ac45cce95f7031c9e

Further refinements may still be needed as problems arise during implementation.

#19 Updated by Junxiao Shi over 4 years ago

  • Blocks Task #2874: Decide directory structure for NDNLPv2 added

#20 Updated by Junxiao Shi over 4 years ago

#21 Updated by Junxiao Shi over 4 years ago

  • Blocks Feature #2876: NDNLPv2: simple value type declaration added

#22 Updated by Junxiao Shi over 4 years ago

#23 Updated by Junxiao Shi over 4 years ago

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/92c1ad907c314727ad2af31ba4b7aa978d077f30

FaceId type is eliminated in favor of using uint64_t directly.

#24 Updated by Junxiao Shi over 4 years ago

  • Subject changed from Design NDNLPv2 API: NACK, Fragmentation, NextHopFaceId, CachingPolicy, IncomingFaceId to NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachingPolicy, IncomingFaceId

Renaming the issue to clarify that this design has only covered packet format API.

#25 Updated by Junxiao Shi over 4 years ago

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/437a1dcac0ec708bd5e52610ffcffe753e572be8

  • major change around NackReason to resolve #2877 note-11 note-13 issues
  • divide Nack into NackHeader and Nack classes: NackHeader only contains the header field, Nack binds a header field and an Interest for use in network layer

#26 Updated by Davide Pesavento over 4 years ago

While I agree in general to dropping the Ndnlp and Nack prefix/suffix from most names, now I fear that the classes Duplicate and GiveUp became too general... maybe add a Reason suffix or something like that?

#27 Updated by Junxiao Shi over 4 years ago

  • Description updated (diff)

Answer to note-26:

It's unnecessary. Duplicate and GiveUp are unlikely to have a clash with another NDNLPv2 concept.

#28 Updated by Davide Pesavento over 4 years ago

Do we really need all this std::integral_constant<uint64_t, ...> stuff? why isn't a static constexpr uint64_t ... enough?

#29 Updated by Junxiao Shi over 4 years ago

Answer to note-28:

Having a static constant field requires the field to be declared in a .cpp, but it's sometimes desirable to avoid the .cpp.

Using the std::integral_constant template looks better than declaring a static method.

#30 Updated by Junxiao Shi over 4 years ago

  • Status changed from Closed to In Progress
  • % Done changed from 100 to 80

#31 Updated by Junxiao Shi over 4 years ago

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/e120cac693cb2e21756a1f2d059e1d5e325d283a

This revision is influenced by http://redmine.named-data.net/issues/2877#note-24, with the following differences:

  • A reason code is represented by an enum class rather than an integer. It's better to use a strong type.
  • nackInfo API is not defined because it's not used by Duplicate and GiveUp.

#32 Updated by Junxiao Shi over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 80 to 100

#33 Updated by Alex Afanasyev over 4 years ago

I don't really like that you completely removed NackReason-specific elements in the spec. Why do you want to modify core parts of the specification (i.e., suddenly adding rules that Nack can actually contain multiple elements and define rules how to process those elements) instead of defining it just once upfront?

When we have the definition in the spec, when we have a new reason with additional elements, we would only need to do a minor modification of the specification and (if we implement in generalized way with NackInfo API as I suggested) we wouldn't even need to modify anything in the library.

#34 Updated by Eric Newberry over 4 years ago

Why does NackHeader::setReason() return a Nack? This doesn't make sense to me, as the Nack object also contains an Interest. Without specifying an Interest to this function, the Nack would have an empty Interest.

#35 Updated by Junxiao Shi over 4 years ago

Why does NackHeader::setReason() return a Nack?

It's a typo due to earlier renaming from Nack to NackHeader.

This is fixed in revision 0abc0f7da30c54014934c54843f32e5c2612aef7.

#36 Updated by Junxiao Shi about 4 years ago

When reviewing ndn-cxx:commit:29c6b361eb981c76ef3086d2fd8520bb952bc517, I noticed an unintentional consequence of declaring TLV-TYPE codes in ndn::lp::tlv namespace: NDNLP implementation code cannot reference ndn::tlv members with tlv::.

The following snippet does not compile:

namespace ndn {
namespace lp {
namespace tlv {

enum {
  X = 3
};

} // namespace tlv

void
demo()
{
  int a = tlv::Interest; // needs to be ndn::tlv::Interest
  a += tlv::X;
  throw tlv::Error(); // needs to be ndn::tlv::Error
}

} // namespace lp
} // namespace ndn

Shall I change the namespace back to ndn::tlv::lp?

ndn::lp::tlv alias is also disallowed, otherwise the same problem would occur.

#37 Updated by Junxiao Shi about 4 years ago

  • Project changed from NFD to ndn-cxx
  • Description updated (diff)
  • Category changed from Protocol to Base
  • Target version changed from v0.4 to v0.4

#38 Updated by Eric Newberry about 4 years ago

RE note-36:

I would favor going back to ndn::tlv::lp. It fits with the style of the other ndn-cxx TLV namespaces, such as ndn::tlv::nfd and ndn::tlv:security.

#39 Updated by Alex Afanasyev about 4 years ago

I prefer the way it is done currently. Explicitly saying ndn::tlv::Error is an indication that it is related to general tlv. If you don't like typing, we can import some of the common elements to ndn::lp::tlv (e.g., Error), but I don't think it is necessary.

I could be ok moving everything to ndn::tlv namespace, but it has its own problems and consequences, which I don't really like.

#40 Updated by Junxiao Shi about 4 years ago

  • Status changed from Resolved to In Progress

The consequence of changing to ndn::tlv::lp would be: NDNLP TLV-TYPE codes must be referenced like tlv::lp::NackReason.

Thus, I decide not to make note-36 change.

I disagree with importing any ndn::tlv member into ndn::lp::tlv, if there's no difference in semantics.


This design will be updated for #2763 note-20 change.

#41 Updated by Junxiao Shi about 4 years ago

  • Status changed from In Progress to Closed

#42 Updated by Davide Pesavento about 4 years ago

  • Subject changed from NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachingPolicy, IncomingFaceId to NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceId

#43 Updated by Eric Newberry about 4 years ago

I believe I found an error at line 414:

template<encoding::Tag TAG>
size_t
CachePolicy(EncodingImpl<TAG>& encoder) const;

Shouldn't this be:

template<encoding::Tag TAG>
size_t
wireEncode(EncodingImpl<TAG>& encoder) const;

#44 Updated by Junxiao Shi about 4 years ago

Answer to note-43: you caught another typo.

It's fixed in https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/05ce27bd979ad26014aa840a973a599d59c05abc, which also adds an example.

#45 Updated by Junxiao Shi about 4 years ago

Revsion 8bf17c1aa7436a2a1103bb452f52a8617bae0169 makes a small change to Field::encode API to have consistent parameter type with wireEncode.

#46 Updated by Junxiao Shi about 4 years ago

  • Description updated (diff)

#47 Updated by Junxiao Shi about 4 years ago

Also available in: Atom PDF