Project

General

Profile

Actions

Feature #2877

closed

NDNLPv2: NackReason abstraction

Added by Junxiao Shi over 9 years ago. Updated over 9 years ago.

Status:
Abandoned
Priority:
Normal
Assignee:
Category:
Base
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
3.00 h

Description

Implement NDNLPv2 functionality.

This issue includes these parts of #2841 API:

  • NackReasonType enum class
  • NackReason base class
  • NackReasonConcrete concept check
  • Duplicate and GiveUp typedefs
  • templates and specializations to support Duplicate and GiveUp typedefs

Related issues 3 (0 open3 closed)

Related to ndn-cxx - Task #2874: Decide directory structure for NDNLPv2ClosedJunxiao Shi

Actions
Blocked by ndn-cxx - Task #2841: NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceIdClosedJunxiao Shi

Actions
Blocks ndn-cxx - Feature #2878: NDNLPv2: Nack abstractionClosedEric Newberry

Actions
Actions #1

Updated by Junxiao Shi over 9 years ago

  • Related to Task #2874: Decide directory structure for NDNLPv2 added
Actions #2

Updated by Junxiao Shi over 9 years ago

  • Blocked by Task #2841: NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceId added
Actions #3

Updated by Junxiao Shi over 9 years ago

Actions #4

Updated by Eric Newberry over 9 years ago

  • Status changed from New to In Progress

What should NackReasonConcrete check for using the concept check?

Actions #5

Updated by Junxiao Shi over 9 years ago

What should NackReasonConcrete check for using the concept check?

  • T is a subclass of NackReason.
  • T::TlvType is a specialization of std::integral_constant<uint64_t, ..>.
Actions #6

Updated by Eric Newberry over 9 years ago

Do I need to implement the Duplicate and GiveUp classes or just create headers for them?

Actions #7

Updated by Junxiao Shi over 9 years ago

Do I need to implement the Duplicate and GiveUp classes or just create headers for them?

According to design, Duplicate and GiveUp are typedefs that use NackReasonEmptyElement template.

They do not have implementation, but need unit testing.

NackReasonEmptyElement template needs implementation (in the header file), but does not need unit testing.

Actions #8

Updated by Eric Newberry over 9 years ago

Do any of the components in this task need implementation in a *.cpp file or just headers?

Actions #9

Updated by Eric Newberry over 9 years ago

Also, for Duplicate and GiveUp, what should I test? I assume the functions, like wireEncode(), wireDecode(), getType()?

Actions #10

Updated by Junxiao Shi over 9 years ago

Do any of the components in this task need implementation in a *.cpp file?

Non-virtual methods of NackReason base type go into .cpp

for Duplicate and GiveUp, what should I test?

Test all methods.

Encoded octets should be compared to hard-coded reference octets, to detect unexpected encoding changes in the future.
Look at test suite of any TLV abstraction type to see how this is done.

Actions #11

Updated by Eric Newberry over 9 years ago

When I attempt to compile, I recieve this error:

error: templates may not be 'virtual'

about template<typename Encoder> virtual size_t NackReason::wireEncode(Encoder& encoder) = 0

In addition, I'm not certain how to implement NackReason::getType(), as the type of the Nack is defined in a subclass and not in NackReason. Apologies if this is simple or doesn't make sense, I'm new to templates and many of the more advanced features of C++.

Actions #12

Updated by Junxiao Shi over 9 years ago

error: templates may not be 'virtual'

I didn't know C++ has this limitation.
I'll see how API design can be refined to make this work.

Actions #13

Updated by Eric Newberry over 9 years ago

Another part of the spec that I don't understand is NackReason::getType(). How does this function return the NackReasonType from NackReason when the type is only declared in NackReasonEmptyElement?

Actions #14

Updated by Eric Newberry over 9 years ago

The same goes for NackReason::wireEncode(). There's no protected Block member to encode (if I'm understanding the ndn-cxx api correctly).

Actions #15

Updated by Junxiao Shi over 9 years ago

The same goes for NackReason::wireEncode(). There's no protected Block member to encode (if I'm understanding the ndn-cxx api correctly).

wireEncode() is now moved to subclasses.

When there's no Block m_wire member field, Block wireEncode() works but const Block& wireEncode() doesn't.
WireEncodable concept accepts either.

Actions #16

Updated by Junxiao Shi over 9 years ago

  • Description updated (diff)
Actions #17

Updated by Eric Newberry over 9 years ago

How do wireEncode and wireDecode work? What are they encoding and how are they encoding it?

Actions #18

Updated by Junxiao Shi over 9 years ago

How do wireEncode and wireDecode work? What are they encoding and how are they encoding it?

A NackReason concrete class (eg. Duplicate) should encode to and decode from a reason element (eg. Duplicate).

I won't answer "how". Look at any TLV abstraction type (eg. ndn::nfd::FaceStatus) for an example.

Actions #19

Updated by Eric Newberry over 9 years ago

  • Status changed from In Progress to Code review
Actions #20

Updated by Alex Afanasyev over 9 years ago

The current implementation http://gerrit.named-data.net/#/c/2123/8 is way over-complicated.

There should be just a single header/implementation file (nack.hpp/nack.cpp, with some external code definitions in some header).

We should have only one real abstraction: lp::Nack. Within lp::Nack should have access to internal "Block" which would represent a specific nack type; plus some helper methods to set/unset codes.

Actions #21

Updated by Junxiao Shi over 9 years ago

I disagree with note-20.

NackReason abstraction is necessary because a reason element can contain sub elements, and what sub elements are permitted to appear within a reason element differs by the type of this reason element.

Actions #22

Updated by Alex Afanasyev over 9 years ago

What I have suggested does not preclude having sub-elements when/if we need them. What we implementing here should be general enough to not require any additional changes (new elements inside Nack block) to support any new features that we or somebody else wants.

For me, Nack block looks very much like SignatureInfo and should have a similar implementation.

Actions #23

Updated by Junxiao Shi over 9 years ago

I don't fully understand note-22.

Please give a complete API design and example, similar to #2841.

Actions #24

Updated by Alex Afanasyev over 9 years ago

Here is what I would like to have for NACK. This includes a small modification of the specification of the NACK block (https://gist.github.com/cawka/f8bb5f558568f06c78ca/revisions)

enum {
  NackUnknown = 0,
  NackDuplicate = 1,
  NackGiveUp = 2
}

// https://gist.github.com/cawka/f8bb5f558568f06c78ca#network-nack
// Changes: https://gist.github.com/cawka/f8bb5f558568f06c78ca/revisions

/** \brief represents a Network NACK header
*/
class NackHeader
{
public:
  NackHeader();

  NackHeader(const Block& block);

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

  const Block&
  wireEncode() const;

  void
  wireDecode(const Block& wire);

public: // getter/setters

  uint64_t
  getReason() const;

  NackHeader&
  setReason(uint64_t reason);

  std::list<Block>&
  getAllInfo();

  void
  clearAllInfo();

  NackHeader&
  addInfo(const Block&);

  void
  removeInfo(uint32_t type);

  /**
   * @brief Helper t
   */
  template<class T=Block>
  Block
  getInfo(uint64_t type);

private:
  uint32_t m_reason;
  std::list<Block> m_nackInfo;
};
Actions #25

Updated by Eric Newberry over 9 years ago

In regard to note-24:

I like the use of NACK reason codes better than reason elements. They provide for easier expansion in the future, as adding additional reason codes requires less additional coding than reason elements.

However, why is uint64_t used for the getters and setters, but the reason code is stored in a uint32_t? Also, does getAllInfo() get a set of Blocks, one for each existing reason code?

Actions #26

Updated by Junxiao Shi over 9 years ago

Please do not hurry with coding. The design choice needs to be discussed in an NFD call.

Actions #27

Updated by Eric Newberry over 9 years ago

@note 26:

I was giving feedback on the design Alex suggested. I'll wait until a design has been decided upon before I start coding.

Actions #28

Updated by Alex Afanasyev over 9 years ago

For removeInfo/getInfo there should be uint32_t (as it is what we are using for TLV type), so I made a mistake here. For reason, I would also use 32-bit, but I don't have strong preferences.

Actions #29

Updated by Junxiao Shi over 9 years ago

  • Status changed from Code review to Abandoned

The intention of nesting suggestion element*s under a *reason element is because a suggestion only makes sense under a specific reason.

But given SignatureInfo does not nest, but appends SignatureType-specific blocks afterwards, I agree with the packet format change.

I no longer demand the design choice to be discussed in an NFD call.

Regarding note-24 API: I agree that after the packet format change, NackReason abstraction is no longer needed.

I do not fully agree with the "nackInfo" API. But since Duplicate and GiveUp reasons do not use it, I would leave this part of the API for a future task.

I'm reopening #2763 #2841 to make changes to the design.

#2877 is abandoned because NackReason abstraction is no longer needed, and everything will be under #2878 Nack abstraction.

Actions

Also available in: Atom PDF