Project

General

Profile

Task #2107

Refactor management system

Added by Junxiao Shi almost 5 years ago. Updated almost 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Management
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
30.00 h

Description

Refactor management system so that:

  • Each Interest is dispatched only once.
  • Authentication is centralized and consistent.
  • Reply encryption can be turned on when necessary.
  • Basic mechanisms can be reused inside and outside NFD.
20150908190620.tgz (145 KB) 20150908190620.tgz integ 2434,1 Junxiao Shi, 09/08/2015 01:30 PM
20150909014411.tgz (149 KB) 20150909014411.tgz Alex Afanasyev, 09/08/2015 11:32 PM

Related issues

Related to NFD - Feature #2182: InMemoryStorage for managementClosed

Related to NFD - Task #2200: Design dispatch mechanism for ManagementClosed

Blocks NFD - Task #2144: Delete NotificationStreamClosed

Blocks NFD - Task #2857: Refactor RibManager to use ManagementDispatcherClosed

Blocks NFD - Feature #3029: Publish RouterName in ForwarderStatusNew

Blocks NFD - Task #3081: Transition ForwarderStatus single packet to GeneralStatus datasetClosed

History

#1 Updated by Junxiao Shi almost 5 years ago

Eliminate double-dispatch describes the design intention of NFD Management system, which can achieve the first three goals.

Since basic mechanisms are increasingly reused in other projects, and some are available from the library, it's also desirable to make the underlying Face compatible with both nfd::Face and ndn::Face, so that code can be reused.

I'll present the basic idea sometime later.

#2 Updated by Anonymous almost 5 years ago

  • Assignee set to Anonymous

#3 Updated by Junxiao Shi almost 5 years ago

@Steve, don't rush on implementation.

There are design issues to solve: goal 4 is incompatible with the design in Eliminate double-dispatch.

#4 Updated by Junxiao Shi almost 5 years ago

  • Blocks Task #2144: Delete NotificationStream added

#5 Updated by Junxiao Shi almost 5 years ago

  • Target version set to v0.3

At 20141112 conference call, Steve agrees to implement this in v0.3, if the design issue can be resolved.

#6 Updated by Junxiao Shi almost 5 years ago

#7 Updated by Junxiao Shi almost 5 years ago

  • Blocks Feature #2184: faces/enable-local-control: CachingPolicy added

#8 Updated by Junxiao Shi almost 5 years ago

  • Blocked by Task #2200: Design dispatch mechanism for Management added

#9 Updated by Junxiao Shi over 4 years ago

  • Estimated time changed from 12.00 h to 30.00 h

This is a large and complex Task.

To ensure success, I recommend the assignee to write all the headers first and submit them for review, and work on the implementation after header review is passed.

I'm increasing the time estimation because I realize that there's also major problems in the unit testing of NFD Management.

There are large amounts of code duplication in the test suite. The test suite needs to be consolidated so that they are easier to maintain.

#10 Updated by Anonymous over 4 years ago

  • Status changed from New to In Progress

#11 Updated by Anonymous over 4 years ago

  • Status changed from In Progress to Feedback
  • % Done changed from 0 to 10

Header changes submitted.

#12 Updated by Anonymous over 4 years ago

  • Status changed from Feedback to In Progress

#13 Updated by Anonymous over 4 years ago

I have a lot of other things going on right now (especially with the run up to the retreat). I do not plan to resume work on this task until after the retreat.

I would prefer to refactor the unit tests as a separate task with a later version target (a v0.3 minor release?) so that I can get through the other v0.3 management tasks. I haven't had a chance to look over the tasks and no refactoring plan has been proposed.

#14 Updated by Junxiao Shi over 4 years ago

I would prefer to refactor the unit tests as a separate task with a later version target (a v0.3 minor release?) so that I can get through the other v0.3 management tasks. I haven't had a chance to look over the tasks and no refactoring plan has been proposed.

Refactoring management system involves changes to class interfaces, which inevitably requires changes to unit tests. Therefore, unit testing refactoring has to be in this Task as well.

I will take more care on test cases in code reviews.
Test cases must be well designed, and code duplication should be avoided.

#15 Updated by Anonymous over 4 years ago

What's the recommended way to test sending Interests and receiving Data in ndn-cxx? Specifically, I want to test ManagementDispatcher::addControlCommand and making sure the handler gets called.

I'm looking at DummyClientFace, but I don't think it does what I want.

#16 Updated by Alex Afanasyev over 4 years ago

DummyClientFace will do everything what you need. You can emulate reception of external interests/data using DummyClientFace::receive methods.

For simple example, you can check https://github.com/named-data/ndn-cxx/blob/master/tests/unit-tests/test-face.cpp.

For complex example, check https://github.com/named-data/NFD/blob/master/tests/daemon/fw/topology-tester.hpp

#17 Updated by Anonymous over 4 years ago

I've looked over the examples, but I'm still stuck. For example, the dispatcher needs to call Face.setInterestFilter to register prefixes. However, I think this needs the io service to run for a bit, otherwise the filter table will be empty when I call receive in the test case. I know NFD has LimitedIo for this sort of thing, but is there something equivalent in cxx? (...or am I overcomplicating things?)

#18 Updated by Junxiao Shi over 4 years ago

I think this needs the io service to run for a bit, otherwise the filter table will be empty when I call receive in the test case. I know NFD has LimitedIo for this sort of thing, but is there something equivalent in cxx?

io.poll()

#19 Updated by Yanbiao Li over 4 years ago

  • Assignee changed from Anonymous to Yanbiao Li

#20 Updated by Alex Afanasyev over 4 years ago

I saw http://gerrit.named-data.net/#/c/2048. Why is it part of ndn-cxx library? Are we expecting this management dispatcher to be used anywhere else except NFD?

#21 Updated by Yanbiao Li over 4 years ago

I guess this is one of the design objectives.

"Basic mechanisms can be reused inside and outside NFD."

#22 Updated by Junxiao Shi over 4 years ago

Are we expecting this management dispatcher to be used anywhere else except NFD?

The intention is to allow using ManagementDispatcher in any application that adopts a management protocol similar to NFD Management.

As pointed out in #2200 note-30 item 1, this goal isn't fulfilled in current design.
I'm thinking about a solution.

#23 Updated by Junxiao Shi over 4 years ago

  • Blocks Task #2857: Refactor RibManager to use ManagementDispatcher added

#24 Updated by Junxiao Shi over 4 years ago

  • Blocks deleted (Feature #2184: faces/enable-local-control: CachingPolicy)

#25 Updated by Junxiao Shi about 4 years ago

  • Blocks Feature #3029: Publish RouterName in ForwarderStatus added

#26 Updated by Junxiao Shi about 4 years ago

  • Target version changed from v0.3 to v0.4

At 20150710 conference call, @Yanbiao reveals that Dispatcher is mostly finished, but he has no idea on how to proceed with the rest of refactoring.

I also don't have sufficient understanding on how to port Managers to use the Dispatcher.

It's requested that @Steve should explain the existing management system.

#27 Updated by Anonymous about 4 years ago

@Yanbiao, please feel free to send me an email to setup a Skype/hangout time. I'm pretty available tomorrow. Otherwise, we can try sometime during the week. (Anyone else interested is welcome to attend, too...not sure this is worth taking time from an NFD call.)

#28 Updated by Yanbiao Li about 4 years ago

@Yanbiao, please feel free to send me an email to setup a Skype/hangout time. I'm pretty available tomorrow. Otherwise, we can try sometime during the week. (Anyone else interested is welcome to attend, too...not sure this is worth taking time from an NFD call.)

Thanks Steve, I has sent an email to you.

#29 Updated by Junxiao Shi about 4 years ago

One problem in dispatcher implementation is: how to emit error logs.

Background:

  • The dispatcher lives in ndn-cxx library which does not have an logging facility.
  • The dispatcher may be used in various applications that have different logging systems: NFD has NFD_LOG_ERROR macro, NLSR has log4cxx.
  • Some functions are executed from io.run() so they shouldn't throw exceptions unless a fatal error (+ program termination) is desired.

Option 1: use log4cxx

  • benefit: powerful logging facility, used by many projects
  • benefit: reusable among all ndn-cxx components
  • drawback: extra dependency

Option 2: emit an logError signal from Dispatcher

  • benefit: works without dependency
  • drawback: log lines are generated even if no logging facility is connected (note: dispatcher emits logError only in error conditions; it's not for debug logs)

Option 3: have a global logError signal

  • benefit: works without dependency
  • benefit: reusable among all ndn-cxx components
  • drawback: log lines are generated even if no logging facility is connected (no problem with dispatcher, but other components can easily abuse this)

#30 Updated by Junxiao Shi about 4 years ago

20150727 conference call decides:

  • Put a // #define NDN_CXX_MGMT_DISPATCHER_ENABLE_LOGGING at the top of dispatcher.hpp.
  • Dispatcher can write error logs into std::clog.
  • All logging commands should be wrapped in #ifdef NDN_CXX_MGMT_DISPATCHER_ENABLE_LOGGING #endif // NDN_CXX_MGMT_DISPATCHER_ENABLE_LOGGING.

#31 Updated by Junxiao Shi about 4 years ago

Reply to #2200 note-76 item 2:

When publishing face status, we should avoid the old data be fetched if there is a newer version. Shall we do this by setting NO CACHY policy or setting a freshnessPeriod?

In this Task, a FreshnessPeriod consistent with older version should be set.

Setting NO_CACHE would cause StatusDataset with multiple segments to stop working, because there's no internal cache in Dispatcher, and responding to subsequent segments relies on the ContentStore.

#2182 will add an internal cache, and then NO_CACHE will be set.

#32 Updated by Junxiao Shi about 4 years ago

From Alex:

I have briefly checked FibManager, FaceManager, and StrategyChoiceManager test cases. I do not see how they are affected by the refactoring of the management. What is mostly tested there is the effects how manager reacts on the incoming commands: this behavior is not changing.
@Junxiao: what exactly you see the problem with fib-manager.t.cpp, face-manager.t.cpp, strategy-choice-manager.t.cpp? They are not ideal, but I cannot say they completely wrong and unreadable. Most test cases have self-explanatory names; actual checks I would agree are not very straightforward, but they should do the job.

I didn't say the current test cases are wrong. They just have too much duplication and are hard to maintain.

If test cases are not broken in this task, I could agree to split test case improvements into a separate task, but that needs to be done sometime.

From note-9:

There are large amounts of code duplication in the test suite. The test suite needs to be consolidated so that they are easier to maintain.

#33 Updated by Junxiao Shi about 4 years ago

  • Blocks Task #3081: Transition ForwarderStatus single packet to GeneralStatus dataset added

#34 Updated by Junxiao Shi about 4 years ago

At 20150821 conference call, Alex disagrees with deferring unit testing improvements, because existing test cases will be broken when changing into the new dispatcher.

There are two possible options:

  • Delete all test cases, and re-add them.
  • Modify existing test cases BEFORE changing dispatcher, so that they feed Interests into the InternalFace instead of XManager. When changing into the new dispatcher, test cases should not be broken.

Yanbiao is to explore the second option.

#35 Updated by Junxiao Shi about 4 years ago

  • Blocked by deleted (Task #2200: Design dispatch mechanism for Management)

#36 Updated by Junxiao Shi about 4 years ago

  • Related to Task #2200: Design dispatch mechanism for Management added

#37 Updated by Junxiao Shi about 4 years ago

I notice a small issue with Dispatcher design: StatusDatasetContext isn't marked noncopyable. I'll upload another Change to fix this problem, as well as changing some function-by-value to function-by-const-reference.

#40 Updated by Alex Afanasyev about 4 years ago

There is a problem with ForwarderStatusManager and specifically ForwarderStatus dataset.

ForwarderStatus is encoded as

    21 (Content) (size: 75)
      128 (APP_TAG_1) (size: 20) [[0.3.4-commit-c9b26a3]]
      ...
      152 (APP_TAG_1) (size: 1) [[%00]]

and then appended to the context, resulting in two nested Content blocks in the data packet:

  21 (Content) (size: 77)
    21 (Content) (size: 75)
      128 (APP_TAG_1) (size: 20) [[0.3.4-commit-c9b26a3]]
      ...
      152 (APP_TAG_1) (size: 1) [[%00]]

#41 Updated by Alex Afanasyev about 4 years ago

Integration tests are passing now.

#42 Updated by Alex Afanasyev about 4 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 10 to 100

#43 Updated by Alex Afanasyev about 4 years ago

  • Status changed from Code review to New
  • % Done changed from 100 to 80

The code has been merged, but we need to update developer's guide to close this issue.

#44 Updated by Yanbiao Li about 4 years ago

Is this urgent? Since the remote registration also needs to update the Developer's Guide. Can I do their documentation together?

#45 Updated by Junxiao Shi about 4 years ago

  • Status changed from New to Feedback

Don't put this back to New if something is already merged and not reverted.

#46 Updated by Junxiao Shi almost 4 years ago

I have written the documentation of Dispatcher in #2200 note-91.

Yanbiao should write documentation for NFD's usage of Dispatcher as part of this issue; I can't write this part because I'm unfamiliar with Managers.

#47 Updated by Yanbiao Li almost 4 years ago

Where to put this documentation? NFD developer guider?

#48 Updated by Junxiao Shi almost 4 years ago

Answer to note-47:

Dispatcher itself is already documented in ndn-cxx Application Developer Guide.

NFD Management and its usage of Dispatcher should be documented in NFD Developer Guide.

Contact Alex for repository permission.

#49 Updated by Junxiao Shi almost 4 years ago

  • Status changed from Feedback to Closed
  • % Done changed from 80 to 100

NFD Developer Guide has been updated in nfd-docs:commit:08f2271a3ff61acc2737a6449efeddcf9609b9d8.

I also fixed a small mistake in nfd-docs:commit:bed706e5852191e75342e6f30b80a541a6a830b6.

Also available in: Atom PDF