Feature #2182
closedInMemoryStorage for management
100%
Description
Use InMemoryStorage
for StatusDataset and Notification produced by managers, so that they don't rely on ContentStore to accept and not evicting them.
Updated by Junxiao Shi about 10 years ago
- Related to Task #2107: Refactor management system added
Updated by Junxiao Shi about 10 years ago
- Blocks Feature #2181: Disallow unsolicited Data from local apps added
Updated by Junxiao Shi about 10 years ago
- Blocked by Feature #2183: LocalControlHeader: CachingPolicy added
Updated by Alex Afanasyev over 9 years ago
- Target version changed from v0.3 to v0.4
We should start implementation, as if anybody switches to a non-default policy, management could break.
Updated by Alex Afanasyev over 9 years ago
- Blocks Feature #3148: CS policy configuration option added
Updated by Junxiao Shi over 9 years ago
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.
Updated by Junxiao Shi over 9 years ago
- Related to Task #2200: Design dispatch mechanism for Management added
Updated by Junxiao Shi over 9 years ago
20150903 conference call approves the idea in note-6.
Updated by Junxiao Shi about 9 years ago
- Blocked by Feature #3274: InMemoryStorage process MustBeFresh added
Updated by Alex Afanasyev about 9 years ago
- Target version changed from v0.4 to v0.5
Updated by Jeff Burke almost 9 years ago
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?
Updated by Junxiao Shi almost 9 years ago
- Assignee set to Yanbiao Li
@Yanbiao agrees to work on this issue.
This cannot start until #3274 is complete.
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Junxiao Shi almost 9 years ago
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);
Updated by Yanbiao Li almost 9 years ago
- Status changed from New to Code review
- Priority changed from Normal to Urgent
Updated by Yanbiao Li almost 9 years ago
at the very beginning, I did as junxiao suggested in note-14.
Later, I introduced the class SendControlInfo because:
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.
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.
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Alex Afanasyev almost 9 years ago
I agree with Junxiao's proposal. It is much cleaner and simpler than introduction of additional logic and SendControlInfo structure.
Updated by Junxiao Shi almost 9 years ago
- Blocks Bug #1968: nfd-autoreg: Inconsistency between the existing faces and registered prefixes added
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Yanbiao Li almost 9 years ago
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 intoDispatcher
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.
- As you told me. There are still other errors resulted by IPv6.
- 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.
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Yanbiao Li almost 9 years ago
In fact, it's impossible to access
InMemoryStorage
directly from NFD unit testing, because it'sprivate
.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.
Updated by Junxiao Shi almost 9 years ago
- Status changed from Code review to In Progress
note-24 solution is unacceptable. InMemoryStorage should be a private member, not a protected member.
Dispatcher
is a black box and should be tested by expressing Interests to it.
Status is changed to InProgress because another commit is needed but not uploaded.
Updated by Yanbiao Li almost 9 years ago
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
Updated by Junxiao Shi almost 9 years ago
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.
Updated by Junxiao Shi almost 9 years ago
- % Done changed from 0 to 70
Code is merged, but ndn-cxx Application Developer Guide "Management Dispatcher" section needs an update to remove the limitation about relying on ContentStore.
Updated by Junxiao Shi almost 9 years ago
- Blocks deleted (Bug #1968: nfd-autoreg: Inconsistency between the existing faces and registered prefixes)
Updated by Junxiao Shi over 8 years ago
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
I confirm ndn-cxx devguide has been updated correctly.