Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for SVS v2 #14

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Updates for SVS v2 #14

wants to merge 3 commits into from

Conversation

pulsejet
Copy link
Collaborator

@pulsejet pulsejet commented Jan 9, 2024

No description provided.

@yoursunny
Copy link
Member

@justincpresley proposed a more space-efficient encoding that omits the TLV-TYPE and TLV-LENGTH of StateVectorEntry.
I'd suggest incorporate this into the new protocol version.

https://github.com/justincpresley/ndn-sync/blob/606eedea7a20be7502dc474a14685ba8a3d03e93/pkg/svs/state_vector.go#L179-L192

PubSubSpec.md Outdated Show resolved Hide resolved
PubSubSpec.md Outdated Show resolved Hide resolved
PubSubSpec.md Outdated Show resolved Hide resolved
Specification.md Outdated Show resolved Hide resolved
Specification.md Outdated Show resolved Hide resolved
Specification.md Outdated

A state vector is appended to the name in TLV format:
The State Vector is encoded in TLV format and included in the Application Parameters of the Sync Interest. The State Vector SHOULD be the first TLV block in the Application Parameters.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under TLV evolvability guidelines, it's not good to enforce a particular TLV being the first one, without a strong technical reason.
It's better to accept any ordering, with a recommendation that StateVector appearing before MappingData.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a recommendation that StateVector appearing before MappingData

why the recommendation? unless a specific order is mandatory, a receiver cannot make any assumptions anyway, so why bother with a recommended order in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommended ordering improves Interest aggregation. It is not enforced.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure interest aggregation is a concern here? it's a signed interest anyway...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signing with HMAC key and skipping all random fields in SigInfo => aggregation possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not debating whether it's possible or not, but whether it's a concern. I'm saying "why bother". Moreover, this may even be application dependent, and the order itself (even if just a recommendation) doesn't matter as long as it's consistent.

Copy link
Member

@Pesa Pesa Jan 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: I'm not against suggesting a specific order in SVS, I just don't think it's very useful or makes a meaningful difference in practice. A consistent order across all ApplicationParameters is more important.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't care honestly. But it's easier / better to recommend a order than recommending consistency for interoperability between vendors.

@justincpresley
Copy link

@justincpresley proposed a more space-efficient encoding that omits the TLV-TYPE and TLV-LENGTH of StateVectorEntry. I'd suggest incorporate this into the new protocol version.

https://github.com/justincpresley/ndn-sync/blob/606eedea7a20be7502dc474a14685ba8a3d03e93/pkg/svs/state_vector.go#L179-L192

I discuss this at length here.

In short, I propose an addition to the specification of NDN: making the concept of a TLV List explicit and heterogeneous. My encoding improvements to SVS simply follow this proposal. Changing only SVS could mislead other protocols.

@Pesa
Copy link
Member

Pesa commented Jan 20, 2024

In short, I propose an addition to the specification of NDN: making the concept of a TLV List explicit and heterogeneous. My encoding improvements to SVS simply follow this proposal. Changing only SVS could mislead other protocols.

When you say "specification of NDN", are you referring to the network-layer packet format? Can you make an example where this new encoding could be used?
(this may be a bit off topic)

@justincpresley
Copy link

justincpresley commented Jan 20, 2024

Yes to the packet format. I assume TLV Lists are not heavily used in creating the concept of a NDN packet and thus left unstated.
However it needs to be defined somewhere preferably wherever people learn about the building blocks of NDN.

I suppose just documentation? added to the end of the TLV section or a page like it.

Anywhere where a list is involved. A Name is naturally a heterogeneous list already. Possibly only useful in custom TLVs. Statevectors, mapping info, in ndn-cert when agreeing upon algorithms, etc.

Specification.md Outdated Show resolved Hide resolved
@justincpresley
Copy link

My thoughts for v2:

  • Rather than a vector of [NodeID : Seqno], I believe a more NDN-inspired approach would be a vector of [Dataset : Seqno]. This change allows a node to control/update multiple datasets within SVS. While you can achieve this with PubSub, I find this extremely inefficient. If you already working with sequential data, there should not be encapsulation for the purpose of removing the sequential part, only to encapsulate sequential data. It seems silly.
    With this change, wording needs to be corrected across the board, I have seen participant/source/nid/nodeID throughout the different papers and repos.

  • Suppression Curve. While this is not complete as ndn-svs states it will later be adjusted based on the number of Nodes, I believe it should still be included in some way.

  • Space-efficient Encoding. I do not have a preference whether this is added or not. Even if it is not added, it can still be provided by libraries as a option (not enforced).

I am a fan of many smaller revisions/releases like NFD has had. This especially helps in this case as many libraries implement this protocol, and it gives them time to adopt changes. I think the partial StateVector work could be included in v3 if it does not make it in v2.

@pulsejet pulsejet force-pushed the svsv2 branch 3 times, most recently from 88acde2 to d173e06 Compare March 8, 2024 16:01
@pulsejet
Copy link
Collaborator Author

pulsejet commented Mar 8, 2024

The suppression curve is now part of the spec.

I prefer not compromising soundness of structure for space efficiency. In fact the first version of SVS used a more efficient encoding but it's more complicated to parse and evolve.

Specification.md Outdated Show resolved Hide resolved
Specification.md Show resolved Hide resolved
@zjkmxy
Copy link
Member

zjkmxy commented Mar 17, 2024

How about we fix the state diagram as well?

Specification.md Outdated Show resolved Hide resolved
Specification.md Outdated Show resolved Hide resolved
Specification.md Outdated Show resolved Hide resolved
@justincpresley
Copy link

When entering suppression state, is it still relevant to check against time remaining?
i.e. in the diagram min(remaining_time, update_delay())

I would think no right? due to the suppression curve change.

Copy link
Member

@yoursunny yoursunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to update the date in PubSubSpec.md.


To optimize the delivery of mapping data, SVS-PS implementations MAY piggyback mapping data in the Sync Interest's `ApplicationParameters`. If present, the mapping data SHOULD be appended as a single block after the StateVector in the `ApplicationParameters`. The mapping data MUST be encoded as a TLV block of type `MappingData` as defined above.

When this optimization is implemented, Mapping Data SHOULD be inserted in every outgoing Sync Interest sent as a result of new data production. On receiving a Sync Interest, this mapping data can be utilized only after the Interest signature has been validated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, MappingData is generated in response to a query Interest, which includes low-seq and high-seq fields.
With delivery optimization, how could the publisher predict what low-seq and high-seq is wanted by the subscriber?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants