Project

General

Profile

Feature #2734

SegmentFetcher: asynchronous Data validation with Validator

Added by Jeff Thompson almost 4 years ago. Updated over 2 years ago.

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

100%

Estimated time:
5.00 h

Description

SegmentFetcher::fetch uses a verifySegment callback which returns a Data validation result synchronously.
But verifying a Data packet may require network operations to fetch certificates.

SegmentFetcher should accept a Validator instance, and use its asynchronous validation API.

SegmentFetcher is a simple procedure that fetches one segment at a time.
To keep it simple, when validation is in progress, the next segment need not be fetched.


Related issues

Blocks NDN-CCL - Task #2543: Port SegmentFetcherClosed2015-02-19

Blocks NLSR - Task #2965: LSAs should be segmented before they are publishedClosed2015-06-26

Blocks ndn-cxx - Task #3334: Deprecate Face::expressInterest non-Nack overloadsClosed

History

#1 Updated by Jeff Thompson almost 4 years ago

#2 Updated by Junxiao Shi almost 4 years ago

  • Tracker changed from Feature to Bug
  • Subject changed from Should SegmentFetcher.VerifySegment be async? to SegmentFetcher::VerifySegment should be asynchronous
  • Description updated (diff)
  • Category set to Utils
  • Estimated time set to 3.00 h

#3 Updated by Junxiao Shi almost 4 years ago

#1872 is changing the API of OnDataValidationFailed.
It's recommended to wait until #1872 closes, in order to avoid code conflicts.

#4 Updated by Alex Afanasyev almost 4 years ago

I would pose this issue in a slightly different terms. Why don't we plug in Validator (require Validator instance to be passed) into the fetcher, instead of callbacks?

#5 Updated by Lan Wang over 3 years ago

We are using SegmentFetcher to implement an application that retrieves NLSR status dataset. This feature is very important for our implementation.

#6 Updated by Alex Afanasyev over 3 years ago

  • Target version set to v0.4

I would not block on #1872. If #1872 is implemented after this issue, then it would simply update what is necessary in SegmentFetcher code and dependencies.

I would say that we should prioritize this issue, as it is important and relatively straightforward to implement. Fetching is already an async process and it was a mistake for VerifySegment by synchronous.

#7 Updated by Junxiao Shi over 3 years ago

  • Subject changed from SegmentFetcher::VerifySegment should be asynchronous to SegmentFetcher should allow asynchronous Data validation
  • Description updated (diff)

20150810 conference call approves the idea in note-4.

#8 Updated by Junxiao Shi over 3 years ago

  • Blocks Task #2965: LSAs should be segmented before they are published added

#9 Updated by Junxiao Shi over 3 years ago

  • Estimated time changed from 3.00 h to 5.00 h

I'm increasing estimated time because using Validator needs more work in changing unit testing.

#10 Updated by Junxiao Shi over 3 years ago

  • Tracker changed from Bug to Feature
  • Subject changed from SegmentFetcher should allow asynchronous Data validation to SegmentFetcher: asynchronous Data validation with Validator
  • Start date deleted (04/07/2015)

#11 Updated by Alex Afanasyev over 3 years ago

  • Assignee set to Muktadir Chowdhury

#12 Updated by Junxiao Shi over 3 years ago

  • Status changed from New to Code review

http://gerrit.named-data.net/2418

commit:a612b66d6573e5b5805ee74094aa784de82d287f changes SegmentFetcher public API as:

static void fetch(Face& face, const Interest& baseInterest, Validator& validator, const CompleteCallback& completeCallback, const ErrorCallback& errorCallback);

One question was:

Since the changes to other projects require maintaining a ValidatorNull, should the SegmentFetcher class instead provide a static ValidatorNull that users can pass to fetch()?

While I won't comment specifically on ValidatorNull, this is a general problem: the caller is forced to maintain a Validator instance.

One solution is: add another fetch overload that accepts shared_ptr<Validator> in place of Validator&.
Therefore, if the caller is unwilling to maintain the Validator instance, it could use this overload and let the SegmentFetcher keep the Validator alive.

#13 Updated by Alex Afanasyev over 3 years ago

I'm not against adding an overload, but what if we simply change fetch signature to accept shared_ptr<Valiator>? Would there be any negative consequences?

#14 Updated by Junxiao Shi over 3 years ago

Answer to note-13:

Only accepting shared_ptr<Validator> forces the caller to create Validator with make_shared, which is undesirable.

Many classes have been accepting Validator& as a constructor parameter, and .shared_from_this() imposes additional requirements on the caller.

#15 Updated by Muktadir Chowdhury over 3 years ago

There are four kinds of nack.

if (nack == NONE) {
    reexpress the interest;
}
else if (nack == CONGESTION) {
    reexpress the interest after waiting a while;
}
else if (nack == DUPLICATE) {
    wait for timeout/data;
}
else if (nack == NO_ROUTE) {
     error;
}

#16 Updated by Junxiao Shi over 3 years ago

note-15 design is incomplete:

  • "NONE" is not a NackReason. It indicates a Nack with no reason.
  • Application SHOULD treat unrecognized NackReason as a Nack with no reason.
  • After DUPLICATE, neither timeout nor Data would come back.

Recommendation:

switch (nack.getReason()) {
case DUPLICATE:
  re-express without delay, up to 3 times;
  break;
case CONGESTION:
  re-express with exponential back-off, up to 3 times;
  break;
default:
  fail;
}

#17 Updated by Junxiao Shi over 3 years ago

How do we coordinate this commit to be merged?

This is a breaking change, so it requires a 5-day notice to ndn-lib mailing list.

Also, prepare patches for NFD, NLSR, and other projects.

ndn-cxx-breaks can help with this process.

#18 Updated by Junxiao Shi over 3 years ago

  • Blocks Task #3334: Deprecate Face::expressInterest non-Nack overloads added

#19 Updated by Muktadir Chowdhury over 3 years ago

#20 Updated by Vince Lehman over 3 years ago

I can confirm that ChronoChat (http://gerrit.named-data.net/#/c/2584/2) compiles with this ndn-cxx patch (http://gerrit.named-data.net/#/c/2418/26).

laptop:ndn-cxx vince$ grep BUILD_STRING /usr/local/include/ndn-cxx/version.hpp | tail -n 1
#define NDN_CXX_VERSION_BUILD_STRING "0.1.0-371-gf9f5dc1"
…
laptop:ChronoChat vince$ git rev-parse HEAD
5119f3d84a8abcd7c538f9216587a12ea65b48b9
…
laptop:ChronoChat vince$ ./waf
…
[73/75] Linking build/ChronoChat
[74/75] Compiling build/ChronoChat
[75/75] Creating build/ChronoChat.app/Contents/Info.plist
'build' finished successfully (44.015s)

#21 Updated by Alex Afanasyev over 3 years ago

  • Status changed from Code review to Closed
  • % Done changed from 0 to 100

#22 Updated by Junxiao Shi over 2 years ago

  • Status changed from Closed to Feedback

The test suite introduced with this feature is running slowly on my machine.
https://gerrit.named-data.net/2926 improves it.

#23 Updated by Junxiao Shi over 2 years ago

Test duration comparison:

http://jenkins.named-data.net/job/ndn-cxx/3596/OS=Ubuntu-14.04-64bit/consoleText -> http://jenkins.named-data.net/job/ndn-cxx/3599/OS=Ubuntu-14.04-64bit/consoleText

test case old duration new duration
Timeout 4373 2172
Triple 2345 1370
MultipleSegmentFetching 173322 57522
SegmentZero 38669 1535

#24 Updated by Junxiao Shi over 2 years ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF