NFD

The difficulty we met during initial LpFace implementation was:

• Each of Transport and LinkService provides some counters.
• Face (aka LpFace) should collect counters from both Transport and LinkService.
• The FaceCounters class inherits from both LinkLayerCounters and NetworkLayerCounters, so the counters from Transport and LinkServices would need to be copied to FaceCounters.
• But a counter is noncopyable.

I have two solutions.

SOLUTION 1

• Transport and LinkService each has its own set of counters.
• FaceCounters is a composition of Transport's counters and LinkService's counters. It contains a pointer to each set of counters.
• benefit: An individual Transport/LinkService can potentially provide additional counters than the standard ones. They can be accessed in simulations, etc.

SOLUTION 2

• All counters are in the Face.
• Transport and LinkService can increment a counter through .getFace accessor
• drawback: Transport/LinkService depends on Face for counters. Among others, this complicates unit testing.
• drawback: Transport/LinkService cannot extend other counters.

I like the first approach more than the second approach. However, why do we need counters in the Face, in addition to the Transport and LinkService?

I'm against solution 2.

Solution 1 sounds acceptable, but I'm wondering why we need this "composition" of counters in the face. If needed, the face can simply be a façade for LinkService+Transport counters.

I'm against solution 2 as well. Solution 1 could be acceptable for now, but in the long-term we should design a flexible counters: when we publish counters as part of status dataset, different combination of transport/link service would potentially give different set of counters.

I do not like solution 2 as well.

Solution 1 seems better, however, I have the same question with Davide:

Why do we need this "composition" of counters in the face?

The idea of having counters in Face is to hide the LinkService+Transport structure from management.

Management should be able to pull counters (and also properties) from Face in order to populate datasets, without knowing the internal structure of Face.

Exactly, so what's the problem with having one or more façade methods in LpFace for the LinkService+Transport counters? We already have a similar thing for the scope, persistency, link type, etc...

Answer to note-10:

If Face class have .getLinkServiceCounters() .getTransportCounters(), it's still exposing the LinkService+Transport internal structure to management.

Face class should a single .getCounters().
The return value of this function may have getters like .getNInInterests() .getNInBytes() that call the corresponding getters in TransportCounters LinkServiceCounters.

Junxiao Shi wrote:

Face class should a single .getCounters().
The return value of this function may have getters like .getNInInterests() .getNInBytes() that call the corresponding getters in TransportCounters LinkServiceCounters.

Yes, this is what I meant. The wrapper returned from getCounters() should be as thin as possible though.

The semantics of byte counters was defined in #1729 as:

Byte count field reflect number of bytes received or sent on link layer. Those counters include link layer headers imposed by NFD (NDNLP or LocalControlHeader), but exclude headers of underlying protocol (Ethernet or TCP or UDP).

To fulfill this semantics, we could increment the byte counters at Transport::send and Transport::receive.

In the design, there's also a "transport packet counter" that should be incremented at the same time.

The concern is: should the byte counters be incremented when the transport is DOWN?

For RECEIVE counters, there's little difference.

For SEND counters:

• Transport::send can still be used to attempt sending a packet even if the transport is DOWN.
• For UDP or Ethernet transport, the packet is still sent through the socket, and still consumes bandwidth.
• For TCP transport, the packet is definitely dropped when TCP connection is not established, and thus does not consume bandwidth.

If we would like to differentiate, the counter should not be incremented in Transport::send.

Transport could expose a method that subclass should invoke after a packet is sent onto the network, which increments the counter.

Junxiao Shi wrote:

• For UDP or Ethernet transport, the packet is still sent through the socket, and still consumes bandwidth.

does it? I guess it depends on how you define bandwidth... but if I unplug my ethernet cable I'm definitely not consuming bandwidth...

If we would like to differentiate, the counter should not be incremented in Transport::send.

Transport could expose a method that subclass should invoke after a packet is sent onto the network, which increments the counter.

I don't know... I'd rather keep a consistent behavior across all transports.

One issue with patchset1 is:

• GenericLinkService::Counters inherits from LinkService::Counters
• LinkService::m_counters has type LinkService::Counters and is returned by LinkService::getCounters() virtual method
• GenericLinkService::m_counters has type GenericLinkService::Counters and is returned by GenericLinkService::getCounters() virtual method which overrides LinkService::getCounters()
• In GenericLinkService instance, the memory for LinkService::m_counters is wasted because it's never used

An alternate is to make LinkService::getCounters() pure virtual, but this forces every LinkService subclass to declare this method, which I want to avoid.

(it's not a problem for LinkService because we only have two in the plan; but Transport::Counters needs the same inheritance capability and there are many transports)

@Davide suggested "diamond inheritance with virtual bases" as a solution but I don't understand.
Can you explain more?

See patch set #2

I disagree with the sample in patchset2 nfd:commit:d1c96431b9f567cccf6ba1a3b5d16350d6b9e1c1

Its basic idea is:

struct LsCounters
{
  counters for LinkService
};

class LinkService : public LsCounters
{
};

struct GlsCounters : public virtual LsCounters
{
  extra counters for GenericLinkService
};

class GenericLinkService : public LinkService, public virtual GlsCounters
{
};

The problem is: inheritance represents an is-a relation, but a LinkService is not a Counters but has-a Counters.

Note: changing into non-public inheritance does not solve this problem.

Junxiao Shi wrote:

The problem is: inheritance represents an is-a relation, but a LinkService is not a Counters but has-a Counters.

Note: changing into non-public inheritance does not solve this problem.

Why do you say that it doesn't solve the "problem"? Only public inheritance establishes an is-a relationship, protected inheritance does not, because it would violate the substitution principle. So effectively the fact that LsCounters is a protected base of LinkService is an implementation detail. Also consider the fact that private and protected inheritance are usually considered much more similar to composition than to type inheritance.

(obviously using protected inheritance requires moving getCounters back into LinkService, but this shouldn't affect the rest of the design)

Answer to note-22:

It seems that I misunderstood the semantics of inheritance.
http://www.cprogramming.com/tutorial/private.html says:

Private inheritance does not model an is-a relationship in the same way that public inheritance does. Instead, private inheritance refers to the idea of being "implemented in terms of a".

Also, CodeStyle does not forbid non-public inheritance.
Thus, I'll adopt patchset2 design but use protected inheritance.

One drawback of virtual inheritance is that you have to use dynamic_cast in place of static_cast...

http://gerrit.named-data.net/2549 patchset4 fulfills note-23 plan.

I'll soon continue onto Transport counters.

In patchset7, GenericLinkService has two drop counters:

• nInParseError: incoming packets dropped before reassembly because it cannot be parsed as LpPacket; e.g. top-level TLV-TYPE is not LpPacket or Interest or Data, FragIndex is greater than FragCount
• nInNetInvalid: a reassembled network layer packet is dropped because an error in network layer; e.g. Interest does not have Name field, Data arrives with Nack field

This distinction is necessary, because they have different granularity: each nInParseError increment drops one LpPacket, while each nInNetInvalid increment can drop multiple LpPackets (all fragments that form this network layer packet).

Junxiao Shi wrote:

The remaining work is to figure out what to do with LpFace::getCounters and LpFaceWrapper::getCounters.

Can you elaborate on what the problem is?

This feature has been split to two commits:

http://gerrit.named-data.net/2549 adds LinkService and Transport counters;

http://gerrit.named-data.net/2563 uses LinkService and Transport counters to compose FaceCounters, replacing old FaceCounters.