-
Notifications
You must be signed in to change notification settings - Fork 162
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
PS,SD: Use new path lookup strategy #2997
PS,SD: Use new path lookup strategy #2997
Conversation
0c55f1a
to
abe4458
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)
go/path_srv/main.go, line 164 at r1 (raw file):
deduper := handlers.NewGetSegsDeduper(msger) if core { segReqHandler = handlers.NewSegReqCoreHandler(args, deduper)
No dedupe 💯
go/sciond/internal/fetcher/fetcher.go, line 64 at r1 (raw file):
revCache revcache.RevCache, cfg config.SDConfig, logger log.Logger) *Fetcher { localIA := itopo.Get().ISD_AS
do not call itopo directly.
go/sciond/internal/fetcher/fetcher.go, line 88 at r1 (raw file):
handler := &fetcherHandler{ Fetcher: f, topology: itopo.Get(),
replace that too,
go/sciond/internal/fetcher/fetcher.go, line 283 at r1 (raw file):
Intfs: intfs, } // this is a bit involved, we have to delete the next query cache,
why can we not just delete all NQ that either start or end with the current IA?
d424140
to
63cae14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 12 of 17 files reviewed, 3 unresolved discussions (waiting on @oncilla)
go/path_srv/main.go, line 164 at r1 (raw file):
Previously, Oncilla wrote…
No dedupe 💯
🎉
go/sciond/internal/fetcher/fetcher.go, line 64 at r1 (raw file):
Previously, Oncilla wrote…
do not call itopo directly.
Done.
go/sciond/internal/fetcher/fetcher.go, line 88 at r1 (raw file):
Previously, Oncilla wrote…
replace that too,
Done.
go/sciond/internal/fetcher/fetcher.go, line 283 at r1 (raw file):
Previously, Oncilla wrote…
why can we not just delete all NQ that either start or end with the current IA?
I tried to do that (if you look into the history) but that isn't enough. For example a down segment can pass through your AS and will be deleted but the next query would still be in there. It actually took me quite some time to figure out what was wrong. The way it is now it should work in any case 🤞 . Tried to improve the comment.
Also hopefully we can remove this once we change the refresh flag (#2871)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved
c735526
to
e48afc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
go/lib/infra/messenger/addr.go, line 126 at r3 (raw file):
newAddr := snetAddr.Copy() defer func() {
why as a defer
and not simple statement?
56520bb
to
f5f0cd5
Compare
Integrates the new path lookup strategy into sciond and PS. This is a breaking change, the PS and sciond have completely different semantics for path lookups. Fixes scionproto#2834 Fixes scionproto#2835 Fixes scionproto#2454
06c3cf6
to
c471fd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 12 of 20 files reviewed, 1 unresolved discussion (waiting on @oncilla)
go/lib/infra/messenger/addr.go, line 126 at r3 (raw file):
Previously, Oncilla wrote…
why as a
defer
and not simple statement?
Because I want to have it printed on every exit path here. (note that newAddr.ǸextHop
is set in the code below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 9 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
Integrates the new path lookup strategy into sciond and PS.
This is a breaking change, the PS and sciond have completely different semantics for path lookups.
Fixes #2834
Fixes #2835
Fixes #2454
This change is