NFD

We should start implementation

I'll provide the design in #2200.

The idea is to integrate the storage as part of the Dispatcher, rather than NFD Management.

as if anybody switches to a non-default policy, management could break.

Yes in general, but no for LRU policy.

20150903 conference call approves the idea in note-6.

This is a solution to the autoregistration issue (see #1968) which is important for proper operation of the testbed. Can it please be assigned for resolution?

Copied from #2200 note-106:

Dispatcher needs to keep InMemoryStorage configurable, but there's no need to expose the capacity configuration to nfd.conf.

For NFD, I suggest setting the capacity to 500, which is enough for the following non-attack condition:

• 50 notifications generated per second
• 5 datasets in active use, 2 versions generated per dataset per second, 20 segments per version
• Data consumed within 2 seconds

Reply to #2200 note-107:

Hard-coded capacity may negatively impact low-memory platforms.

The capacity is a compile-time constant.
Low-memory platforms such as home routers do not use Ubuntu packages, so having a compile-time constant is not a concern.
The constant can be always adjusted to fit the platform during compilation procedure.

Also, I would like to reduce this value when NFD is embedded in ndnSIM.

The capacity can be specified in the top-level initialization code (nfd.cpp) so it's easy to override in ndnSIM.

ndn-cxx:commit:03834d5cc2d8a1af77d08a67090f4ca5169aba9f introduces a SendControlInfo type.

This type is used as an argument Dispatcher::sendData private method, which is invoked when a Data packet is generated for ControlCommand, StatusDataset, or Notification.

This type carries the following fields:

• whether to send the Data through the face
• whether to add the Data into the InMemoryStorage (IMS)
• the FreshnessPeriod to be set on the Data packet
• the duration to keep the Data fresh in IMS

Dispatcher::sendData method is invoked in the following manner:

• ControlCommand
  • the Data is sent through the face but not added to the IMS
  • FreshnessPeriod and IMS fresh duration are kept at default
• StatusDataset
  • the Data is always added to the IMS, is also sent through the face if segment number is zero
  • IMS fresh duration is set to the effect value of StatusDatasetContext::getExpiry
  • FreshnessPeriod is kept at default
• Notification
  • the Data is sent through the face and also added to the IMS
  • FreshnessPeriod and IMS fresh duration are kept at default

I disagree with the introduction of SendControlInfo type because it unnecessarily increases complexity.

• FreshnessPeriod field is completely unused in all three places.
• Whether to send the Data through the face or add it into the IMS are two simple booleans, and can be replaced by a single enum.
• IMS fresh duration is also a simple type, which can be passed as an argument.

In short, I suggest eliminating this type, and changing sendData as:

enum class SendTo { NONE, FACE, IMS, FACE_AND_IMS };
void Dispatcher::sendData(const Name&, const Block&, const MetaInfo&, SendTo, time::nanoseconds imsFresh);

at the very beginning, I did as junxiao suggested in note-14.

Later, I introduced the class SendControlInfo because:

1. In StatusDatasetContext::reject, the SendData method should be invoked as for ControlCommand. It's not required to specify the ExpiryTime. Yes, we can use the default value here, but I thought it's a little bit strange.

2. Currently, we use FreshnessPeriod as a constant. If we want to enable it be configurable for different producers (maybe in the future) or want to amend its constant value, we only need to change the SendControlInfo while leave the Dispatcher as the same.

Anyway, I can change the code back. But I think this is not a big problem. As this issue is urgent, I hope reviewers can pointed out all big problems at one time. Thanks.

In StatusDatasetContext::reject, the SendData method should be invoked as for ControlCommand.

This indicates that it's insufficient to connect a single StatusDatasetContext::DataSender function.
There should be two function pointers, one for send+store, the other for send only.

/**
 */
typedef std::function<void(const Name& name, const Block& content, time::nanoseconds expiryTime, bool isFinalBlock)> SendSegment;
typedef std::function<void(const Name& name, const Block& content)> SendNack;

Currently, we use FreshnessPeriod as a constant. If we want to enable it be configurable for different producers (maybe in the future) or want to amend its constant value, we only need to change the SendControlInfo while leave the Dispatcher as the same.

There's no difference in changing this file or that file.

I agree with Junxiao's proposal. It is much cleaner and simpler than introduction of additional logic and SendControlInfo structure.

ndn-cxx:commit:7911ad3e5b0bdc29151a4cd5bd9be45303061249 breaks NFD.

Before: https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/116731445 (ndn-cxx master, with NFD disabling IPv6 tests)

After: https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/116731575 (ndn-cxx master, with NFD disabling IPv6 tests)

The reason is: only the first segment goes through the face.
so we should modify the ManagerCommonFixture::concatenateResponses method to concatenate responses in a right way.

I disagree with this solution.

Dispatcher should be used as a black box in these test suites.
The test suite should express Interests to retrieve additional segments from the Dispatcher.

but I think it's better to work on it after this commit is merged

No. The patch for NFD should be prepared and tested with ndn-cxx-breaks, before the ndn-cxx Change can merge.

I know this feature 2182 is urgent. And I think it’s ok now.
Following is a issue pointed out by Junxiao. But it’s obviously it’s just a test issue in NFD managers.
I do not understand why the reviewer refuse to approve it.
The problem is:
In current test logic of NFD managers, it’s assumed that all packets sent out by the dispatcher will go through the face.
But due to the use of InMemoryStorage, it’s incorrect now.
So, I think this is a issue on how to redesign the test logic of NFD managers (NFD) but not a issue on the Dispatcher (ndn-cxx).
I’m sure about this and have tested it on my laptop. I’ll submit another commit to resolve this but not with this commit.
This commit just touches the Dispatcher to enable the usage of InMemoryStorage.
In another word, no matter how to redesign the test logic of NFD managers, there will be nothing to do with this commit.
They are separate issues. I can submit a test proving that this issue can be resolved if the test logic of NFD managers is slightly modified.
But how to redesign the test logic of NFD managers finally is another issue. I will resolve it in another commit for NFD. It should not block this commit for ndn-cxx.

If you haven't noticed, #2182 is an NFD issue from the beginning.
The goal is to incorporate InMemoryStorage into NFD Management, and ensure everything including NFD unit tests are working.
Adding InMemoryStorage into Dispatcher is a step toward this goal.

Merging a ndn-cxx Change without preparing a NFD patch that fixes all failing tests would block any further Changes in NFD until that NFD patch arrives, because Jenkins won't build NFD successfully.

If you haven't noticed, #2182 is an NFD issue from the beginning.
The goal is to incorporate InMemoryStorage into NFD Management, and ensure everything including NFD unit tests are working.
Adding InMemoryStorage into Dispatcher is a step toward this goal.

Merging a ndn-cxx Change without preparing a NFD patch that fixes all failing tests would block any further Changes in NFD until that NFD patch arrives, because Jenkins won't build NFD successfully.

1. As you told me. There are still other errors resulted by IPv6.
2. We can solve the issue in test logic in a quick way (because it's just a issue in test, has nothing to do with the NFD management logic) as temp solution, if those errors must be resolved before this feature become available. If we spend another a few days on that issue in discussing how to resolve it, coding and reviewing, this URGNET feature will be blocked.

There are still other errors resulted by IPv6.

IPv6 issues (#3367) affect Travis-CI but not Jenkins.

We can solve the issue in test logic in a quick way (because it's just a issue in test, has nothing to do with the NFD management logic) as temp solution, if those errors must be resolved before this feature become available.

note-20 solution is as quick as your solution.

In fact, it's impossible to access InMemoryStorage directly from NFD unit testing, because it's private. PUBLIC_WITH_TESTS_ELSE_PRIVATE is inapplicable because ndn-cxx is built without tests in an NFD build.

In fact, it's impossible to access InMemoryStorage directly from NFD unit testing, because it's private. PUBLIC_WITH_TESTS_ELSE_PRIVATE is inapplicable because ndn-cxx is built without tests in an NFD build.

yes, I know this. So I made m_storage a protected member and wrote a subclass of Dispatcher for NFD manager test in ManagerCommonFixture which just provides the function of concatenating the packets in in-memory storage.

  auto isFinalSegment = [] (const Data& data) -> bool {
    const auto& segment = data.getName().at(-1);
    if (segment.isSegment() == false) {
      return true;
    }
    return segment == data.getFinalBlockId();
  };

  while (isFinalSegment(m_responses.back()) == false) {
    auto name = m_responses.back().getName();
    auto prefix = name.getPrefix(-1);
    auto segmentNo = name.at(-1).toSegment() + 1;
    receiveInterest(makeInterest(prefix.appendSegment(segmentNo)));
  }

I have added the above code to ManagerCommonFixture::concatenateResponses, and have tested it one my laptop. It works.
So, where to submit this code?

Or, can you merge this commit first. Then, I'll submit a new commit to NFD/master at once

Reply to note-26:

All code should be upload to Gerrit.
If an NFD Change cannot build successfully before an ndn-cxx Change is merged, you can still upload that; Jenkins will report an error, but you may use ndn-cxx-breaks to build with the unmerged ndn-cxx Change.