NFD

Should this be generalized to all NDNLP features, such as fragmentation and reassembly?

Should this be generalized to all NDNLP features, such as fragmentation and reassembly?

Not until there is a use case. Link reliability needs a flag because it has not-trivial overhead (Acks) so it should be enabled only if the link is lossy.

The use case is enabling/disabling fragmentation on UDP tunnels, so that we can use NDNLP fragmentation instead of relying on IP fragmentation.

The use case is enabling/disabling fragmentation on UDP tunnels

Please discuss this use case in #4005.
This issue, #4003, is specifically about enabling link reliability.

A ReliabilityEnabled Flag bitfield has been added to FaceMgmt for faces/create and faces/update.

The ReliabilityEnabled flag has been renamed to LpReliabilityEnabled.

are we allowing to enable LpReliability on any type of face? local faces too?

I think LpReliability should only be allowed on lossy links. We could consider all links as "lossy" unless specified otherwise. Therefore, the following LocalUri schemes are not lossy: unix, tcp4, tcp6, ws.

It has nothing to do with local or non-local. A non-local tcp6 face is still not lossy.

Only allowing reliability to be enabled on lossy transports presents a design issue: how should reliability be enabled on new faces (and validated for appropriateness)?

The current design with enabling local fields adds a parameter to ProtocolFactory::createFace, which then validates the parameter's value according to the type of the factory. However, I'm concerned that we are adding too many parameters to this function, especially if we add additional management flags. Thus I currently see two solutions (there may be more):

• Add the parameter to createFace (which I don't want to do). This also involves needing to update all factory test cases to use the new createFace syntax.

• Create a separate validation function in each factory to validate face flags. This function would be called before the face was created (and the face wouldn't be created if any flags failed validation). The face options would then be adjusted after the face was created, similar to how faces/update works.

The current design with enabling local fields adds a parameter to ProtocolFactory::createFace, which then validates the parameter's value according to the type of the factory.

Actually, the current implementation of BIT_LOCAL_FIELDS_ENABLED is a bit inconsistent. createFace() doesn't validate it (except for a sanity in the success callback), while updateFace() does the validation on its own.

Add the parameter to createFace (which I don't want to do). This also involves needing to update all factory test cases to use the new createFace syntax.

I agree that this approach doesn't scale, thus I would be mildly against it.

Create a separate validation function in each factory to validate face flags. This function would be called before the face was created (and the face wouldn't be created if any flags failed validation). The face options would then be adjusted after the face was created, similar to how faces/update works.

Acceptable. This is also similar to how faces/update handles persistency changes (see canChangePersistencyTo()). However, a major complication is that flag validation may require an arbitrary amount of additional information. In other words, whether a flag value is valid or not does not depend exclusively on the type of factory or face. E.g.: BIT_LOCAL_FIELDS_ENABLED requires checking the face scope.

There's also a third option: instead of passing each ndn::nfd::BIT_* as a separate boolean parameter, pass a single flags parameter to createFace(). This approach alone doesn't solve the validation problem in updateFace() though.

Junxiao Shi wrote:

It has nothing to do with local or non-local. A non-local tcp6 face is still not lossy.

I know, but I don't think we should restrict this setting based on the underlying protocol. If someone wants to enable LpReliability on a TCP link, let them do it.

The rationale for prohibiting it on local faces was that on the other end of a local face there's an application, which doesn't know anything about the LpReliability protocol.

Davide Pesavento wrote:

Junxiao Shi wrote:

It has nothing to do with local or non-local. A non-local tcp6 face is still not lossy.

I know, but I don't think we should restrict this setting based on the underlying protocol. If someone wants to enable LpReliability on a TCP link, let them do it.

I agree with Davide on this one. If reliability is enabled on a non-lossy face, it adds a bit of unnecessary overhead, but it doesn't (shouldn't) break anything.

The rationale for prohibiting it on local faces was that on the other end of a local face there's an application, which doesn't know anything about the LpReliability protocol.

This also makes sense to me. Otherwise, we'd need to implement reliability in the ndn-cxx face as well.

https://gerrit.named-data.net/4029 patchset3 changes ProtocolFactory::createFace function signature with a flags field. This is a bad trend.

• Details of management protocol are going deeper and deeper into face system.
• A widespread change in face system is needed upon each addition to the LinkService. While flags cloud reduce such situations, some future parameters not represented in the flags still require changes.

I'd suggest defining a face creation parameters struct. This struct is part of the face system rather than management. It collects all parameters needed for face creation, and is accepted on ProtocolFactory::createFace and passed down as needed.

struct FaceCreateParams
{
  FaceUri remoteUri;
  optional<FaceUri> localUri;
  FacePersistency persistency;
  bool wantLocalFields;
  bool wantLpReliability;
};

Then, each time adding a new face feature, it may still be necessary to make changes in the Factory-Channel-Transport hierarchy of every time, but the changes are limited to the "verify parameters" function for transports that do not support the new feature.

Note that if this solution is adopted, the refactoring shall be performed in a separate commit before the addition of LpRelibility.

Junxiao Shi wrote:

Note that if this solution is adopted, the refactoring shall be performed in a separate commit before the addition of LpRelibility.

I like this design but I really don't see the need for a separate refactoring commit, since there would be about as many changes (to as many files) as there are in the existing commit.

A refactoring commit is needed so as to highlighting what are the changes to enable reliability.

From change 4029,3:

The scope needs to be determined before it can be tested whether LpReliability can be enabled and, as seen in TcpFactory, there can be some protocol-specific tests that need to be run first. Therefore, we either need to set this parameter after creating the face (as I suggested in an earlier comment on Redmine) or we need to push this logic down into the factory.

Exactly, this is what I was trying to say. What are the pros and cons of each of the two approaches?

Davide Pesavento wrote:

From change 4029,3:

The scope needs to be determined before it can be tested whether LpReliability can be enabled and, as seen in TcpFactory, there can be some protocol-specific tests that need to be run first. Therefore, we either need to set this parameter after creating the face (as I suggested in an earlier comment on Redmine) or we need to push this logic down into the factory.

Exactly, this is what I was trying to say. What are the pros and cons of each of the two approaches?

Let's go through them:

Keeping the system as-is, with the checks pushed down into the specific factories:

• + Face is never considered up with these features either incorrectly enabled/disabled
• - Complicates parameters to createFace (even if they're in a struct, you need to set the values in that struct)

Creating the face with features at their default values and enabling them after creation (and checking the scope):

• + Can eliminate wantLocalFieldsEnabled and wantLpReliabilityEnabled from the createFace method signature
• - If this is a critical feature, the face may be up for a brief period without this feature enabled

ok, so for createFace the first option seems best. What about updateFace?

Davide Pesavento wrote:

ok, so for createFace the first option seems best. What about updateFace?

I don't really have any complaints about the existing implementation of updateFace. As it is, updateFace checks for the appropriateness of enabling local fields given the scope of the existing face, as well as the persistency (through Transport::canChangePersistencyTo). Then, if all changes check out, it sets them to the new values. Otherwise, it fails to change any properties of the face. I can't really think of a better design for this at the moment.

But then the logic will be duplicated, once in updateFace and once somewhere in the factory in the face creation code path. Moreover, if the "appropriateness" check is factory-specific, it cannot be done in FaceManager::updateFace.

Davide Pesavento wrote:

But then the logic will be duplicated, once in updateFace and once somewhere in the factory in the face creation code path. Moreover, if the "appropriateness" check is factory-specific, it cannot be done in FaceManager::updateFace.

The reason we need to move the check into the factories in the first option is because the scope of the face has yet to be determined before the face is created. However, the reason it can be done in updateFace is because, at this point, the scope has already been determined during face creation. FaceManager simply needs to check the scope of the face when validating these options.

Eric Newberry wrote:

The reason we need to move the check into the factories in the first option is because the scope of the face has yet to be determined before the face is created. However, the reason it can be done in updateFace is because, at this point, the scope has already been determined during face creation. FaceManager simply needs to check the scope of the face when validating these options.

I understand that, and I'm not disputing it. But you're missing the point about duplication, and the fact that a future check may be factory-specific, like the persistency check.

Davide Pesavento wrote:

Eric Newberry wrote:

The reason we need to move the check into the factories in the first option is because the scope of the face has yet to be determined before the face is created. However, the reason it can be done in updateFace is because, at this point, the scope has already been determined during face creation. FaceManager simply needs to check the scope of the face when validating these options.

I understand that, and I'm not disputing it. But you're missing the point about duplication, and the fact that a future check may be factory-specific, like the persistency check.

I see what you mean now. In this case, we should probably push this check down into the factory as well. We might even be able to factor it out into a separate function.

20170807 NFD call decides that there's no need to prevent LpReliability enabled on TCP tunnels. Also, there's no need to prevent LpReliability enabled on local faces. This leads to less code.