path lookup: Add request resolver #2909
Conversation
lukedirtwalker
force-pushed
the
pubReqResolver
branch
from
c29b1a7 to
2eae50d
Compare
There was a problem hiding this comment.
Reviewed 4 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, 12 unresolved discussions (waiting on @lukedirtwalker and @oncilla)
go/lib/infra/modules/segfetcher/resolver.go, line 29 at r1 (raw file):
// InvalidRequest indicates an invalid request. const InvalidRequest = "Invalid request set"
the comment and const name indicate request.
the value indicate a set
go/lib/infra/modules/segfetcher/resolver.go, line 31 at r1 (raw file):
const InvalidRequest = "Invalid request set" // Resolver resolves requests in a request set and removes locally cached data.
what does it mean to remove locally cached data?
The comment on DefaultResolver is more clear what happens.
go/lib/infra/modules/segfetcher/resolver.go, line 60 at r1 (raw file):
if !req.Up.IsZero() { segs, req, err = r.resolveUpSegs(ctx, segs, req) if !req.Up.IsZero() || err != nil {
why return on !req.Up.IsZero. It might be that we can resolve some of the down segments.
I guess it should be something like:
if !req.Up.IsZero() {
if segs, req, err = r.resolveUpSegs(ctx, segs, req); err != nil {
return segs, req, err
}
}
if !req.Down.IsZero() {
if segs, req, err = r.resolveDownSegs(ctx, segs, req); err != nil {
return segs, req, err
}
}
if !req.Up.IsZero() || !req.Down.IsZero() {
return segs, req, nil
}that would also make it immediately obvious that req.Up and req.Down are empty when we continue to expand the cores.
go/lib/infra/modules/segfetcher/resolver.go, line 79 at r1 (raw file):
} var cachedReqs Requests remainingCores := req.Cores[:0]
make this until req.Cores = remainingCores a method
go/lib/infra/modules/segfetcher/resolver.go, line 129 at r1 (raw file):
} func (r *DefaultResolver) resolveUpDown(ctx context.Context,
resolveSegments?
go/lib/infra/modules/segfetcher/resolver.go, line 151 at r1 (raw file):
func (r *DefaultResolver) needsFetching(ctx context.Context, dst addr.IA) (bool, error) { nq, err := r.DB.GetNextQuery(ctx, dst)
why does this only consider the destination?
go/lib/infra/modules/segfetcher/resolver.go, line 158 at r1 (raw file):
} func (r *DefaultResolver) expandCores(segs Segments, req RequestSet) ([]Request, error) {
this is super difficult to understand.
split the 3 large cases in different methods?
go/lib/infra/modules/segfetcher/resolver.go, line 159 at r1 (raw file):
func (r *DefaultResolver) expandCores(segs Segments, req RequestSet) ([]Request, error) { coreReq := req.Cores[0]
Wait. Is there always exactly one request in there?
go/lib/infra/modules/segfetcher/resolver.go, line 161 at r1 (raw file):
coreReq := req.Cores[0] switch { case len(req.Cores) > 1:
why?
go/lib/infra/modules/segfetcher/resolver.go, line 165 at r1 (raw file):
case len(segs.Up) == 0 && len(segs.Down) == 0: return req.Cores, nil case len(segs.Up) > 0 && len(segs.Down) == 0:
non-core -> core
this works because len(segs.Down) implies that req.Down was initially zero -> request path to core dst.
It took me some time to figure this out. Please add a comment that states this assumption explicitly.
go/lib/infra/modules/segfetcher/resolver.go, line 178 at r1 (raw file):
} return coreReqs, nil case len(segs.Up) == 0 && len(segs.Down) > 0:
core -> non-core
go/lib/infra/modules/segfetcher/resolver.go, line 191 at r1 (raw file):
} return coreReqs, nil default:
non-core -> non-core
go/lib/xtest/matchers/matchers.go, line 26 at r1 (raw file):
"sort" "github.com/scionproto/scion/go/lib/pathdb/query"
inverted 3party and local
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @lukedirtwalker)
Resolver locally looks up segments it finds and returns a full or partial result of segments. Contributes scionproto#2454
lukedirtwalker
force-pushed
the
pubReqResolver
branch
from
2eae50d to
f714388
Compare
There was a problem hiding this comment.
Reviewed 2 of 5 files at r2.
Reviewable status: 4 of 7 files reviewed, 12 unresolved discussions (waiting on @lukedirtwalker and @oncilla)
There was a problem hiding this comment.
Reviewable status: 3 of 9 files reviewed, 12 unresolved discussions (waiting on @oncilla)
go/lib/infra/modules/segfetcher/resolver.go, line 29 at r1 (raw file):
Previously, Oncilla wrote…
the comment and const name indicate request.
the value indicate a set
Yeah it is really a request the splitting is more of an internal detail of the lib.
go/lib/infra/modules/segfetcher/resolver.go, line 31 at r1 (raw file):
Previously, Oncilla wrote…
what does it mean to remove locally cached data?
The comment on DefaultResolver is more clear what happens.
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 60 at r1 (raw file):
Previously, Oncilla wrote…
why return on
!req.Up.IsZero. It might be that we can resolve some of the down segments.
I guess it should be something like:if !req.Up.IsZero() { if segs, req, err = r.resolveUpSegs(ctx, segs, req); err != nil { return segs, req, err } } if !req.Down.IsZero() { if segs, req, err = r.resolveDownSegs(ctx, segs, req); err != nil { return segs, req, err } } if !req.Up.IsZero() || !req.Down.IsZero() { return segs, req, nil }that would also make it immediately obvious that req.Up and req.Down are empty when we continue to expand the cores.
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 79 at r1 (raw file):
Previously, Oncilla wrote…
make this until
req.Cores = remainingCoresa method
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 129 at r1 (raw file):
Previously, Oncilla wrote…
resolveSegments?
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 151 at r1 (raw file):
Previously, Oncilla wrote…
why does this only consider the destination?
hm yeah this no longer holds. Previously it was a caching for the whole path now it is only on the level of segments.
For up and down segments this is still fine because the Src is always fixed for those requests (or if they can be locally resolved it doesn't really matter)
But for cores it breaks: e.g. a non-core PS receives requests 130 -> 210 and 120 -> 210 this would break horribly.
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 158 at r1 (raw file):
Previously, Oncilla wrote…
this is super difficult to understand.
split the 3 large cases in different methods?
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 159 at r1 (raw file):
Previously, Oncilla wrote…
Wait. Is there always exactly one request in there?
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 161 at r1 (raw file):
Previously, Oncilla wrote…
why?
Done.
go/lib/infra/modules/segfetcher/resolver.go, line 165 at r1 (raw file):
Previously, Oncilla wrote…
non-core -> core
this works because len(segs.Down) implies that req.Down was initially zero -> request path to core dst.
It took me some time to figure this out. Please add a comment that states this assumption explicitly.
Tried to explain assumption at the beginning of the method.
go/lib/infra/modules/segfetcher/resolver.go, line 191 at r1 (raw file):
Previously, Oncilla wrote…
non-core -> non-core
Done.
go/lib/xtest/matchers/matchers.go, line 26 at r1 (raw file):
Previously, Oncilla wrote…
inverted 3party and local
Done.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
go/lib/infra/modules/segfetcher/resolver.go, line 151 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
hm yeah this no longer holds. Previously it was a caching for the whole path now it is only on the level of segments.
For up and down segments this is still fine because the Src is always fixed for those requests (or if they can be locally resolved it doesn't really matter)
But for cores it breaks: e.g. a non-core PS receives requests 130 -> 210 and 120 -> 210 this would break horribly.Done.
It seems like there are 2 queries hidden in there.
- wildcard -> dst
- specific src -> dst
Here we only care about case 2 right?
But are there cases where we care about 1?
go/lib/infra/modules/segfetcher/resolver.go, line 167 at r2 (raw file):
// indicate anything about the location of the local AS. For example after // resolving up segments it could be that the resolver receives a request // core to non-core eventhough it is a non-core AS.
💯
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
go/lib/infra/modules/segfetcher/resolver.go, line 151 at r1 (raw file):
Previously, Oncilla wrote…
It seems like there are 2 queries hidden in there.
- wildcard -> dst
- specific src -> dst
Here we only care about case 2 right?
But are there cases where we care about 1?
For sciond we care about case 1 as well since it starts off with empty DB.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
go/lib/infra/modules/segfetcher/resolver.go, line 151 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
For sciond we care about case 1 as well since it starts off with empty DB.
ack
Resolver locally looks up segments it finds and returns a full or partial result of segments.
Contributes #2454
This change is