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

snet: PathInterface struct #3879

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Sep 17, 2020

Use a simple struct for snet.PathInterface directly instead of an interface.

Contrary to what was implied by code comments, this does not appear to
introduce any problematic dependencies.

Contributes to #3872


This change is Reviewable

@matzf matzf force-pushed the snet-pathinterface-struct branch from e653717 to 80aec0e Compare September 22, 2020 08:00
@matzf matzf marked this pull request as ready for review September 22, 2020 08:00
@oncilla oncilla self-requested a review September 28, 2020 11:25
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 26 of 26 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)


go/cs/segutil/router.go, line 105 at r1 (raw file):

}

type path struct {

AFAIU, there is no longer a need for this struct.

Can you maybe elaborate a bit on the plan for this and future PRs?

I assume this is some preparation PR, and then there will be a follow-up that cleans up the uneccessary structs.
Is that correct?

Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)


go/cs/segutil/router.go, line 105 at r1 (raw file):

I assume this is some preparation PR, and then there will be a follow-up that cleans up the uneccessary structs.

Yup.

In the current state, this is implementation of the Path interface is still used/needed. This PR only removes the PathInterface interface type.

The next step will be to combine the sciond and segutil implementations of path, i.e. the main part of #3872. This is when I will remove this type here.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@oncilla
Copy link
Contributor

oncilla commented Sep 29, 2020

/rebase

Use a simple struct directly instead of an interface.
Contrary to what was implied by code comments, this does not appear to
introduce any problematic dependencies.
@github-actions github-actions bot force-pushed the snet-pathinterface-struct branch from 80aec0e to b1f6e98 Compare September 29, 2020 14:33
@shitz shitz merged commit 962ca85 into scionproto:master Sep 30, 2020
lukedirtwalker pushed a commit that referenced this pull request Nov 5, 2020
Make `snet.PathMetadata` a struct instead of an interface. Analogous to how this was done for `snet.PathInterface` (#3879).
The indirection with an interface is simply unnecessary.
Making this a struct, will allow to extend the `PathMetadata` struct quite easily: in a next PR, we'll add the latency, bandwidth, geo coordinates etc. from the "static info" beaconing extension (currently, these fields are defined in a shadow copy of `PathMetadata` in `combinator/staticinfo_accumulator.go`).

Move `Interfaces` to `snet.PathMetadata`. The `Interfaces` list is available exactly if the other metadata items are available, so it makes sense to bundle these.
This requires/brings some changes to `pathpol.PathSet` -- now the pathpol is fed directly with the `snet.PathMetadata` struct. This seems quite natural, as it's conceivable that the path policy would access all the various metadata items in the future.
This change also requires some minor adjustments at the call sites for the path policies.
@matzf matzf deleted the snet-pathinterface-struct branch November 12, 2020 13:39
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.

3 participants