Feature #2877
closedNDNLPv2: NackReason abstraction
0%
Description
Updated by Junxiao Shi over 9 years ago
- Related to Task #2874: Decide directory structure for NDNLPv2 added
Updated by Junxiao Shi over 9 years ago
- Blocked by Task #2841: NDNLPv2 design: packet format API for NACK, Fragmentation, NextHopFaceId, CachePolicy, IncomingFaceId added
Updated by Junxiao Shi over 9 years ago
- Blocks Feature #2878: NDNLPv2: Nack abstraction added
Updated by Eric Newberry over 9 years ago
- Status changed from New to In Progress
What should NackReasonConcrete check for using the concept check?
Updated by Junxiao Shi over 9 years ago
What should NackReasonConcrete check for using the concept check?
T
is a subclass ofNackReason
.T::TlvType
is a specialization ofstd::integral_constant<uint64_t, ..>
.
Updated by Eric Newberry over 9 years ago
Do I need to implement the Duplicate and GiveUp classes or just create headers for them?
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.
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?
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()?
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.
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++.
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.
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?
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).
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.
Updated by Eric Newberry over 9 years ago
How do wireEncode and wireDecode work? What are they encoding and how are they encoding it?
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.
Updated by Eric Newberry over 9 years ago
- Status changed from In Progress to Code review
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.
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.
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.
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.
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;
};
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?
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.
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.
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.
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.