Reviewed 9 of 10 files at r1.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @chaehni and @lukedirtwalker)

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 33 at r1 (raw file):

const (
	ErrUnknownGroup common.ErrMsg = "group not known to HPS"

Please use serrors.New:

var (
	ErrUnknownGroup = serrors.New("group not known to HPS")
	ErrNotReader    = serrors.New("peer is not a reader of this group")
)

we want to move towards serrors instead of common errors.

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 108 at r1 (raw file):

	endsAt addr.IA, replyChan chan []*path_mgmt.HPSegRecs) {

	// TODO(chaehni): we need to query the DB separately for every GroupId so that we

The query.Result contains a list of hidden path ids can't those be used? If not the code is fine as is also.

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 141 at r1 (raw file):

		GroupIds: rawIds,
	}
	addr := &snet.Addr{IA: remote, Host: addr.NewSVCUDPAppAddr(addr.SvcHPS)}

Realized we need #3245

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 72 at r1 (raw file):

	},
	Version:    1,
	Owner:      ia110,

This mismatches the group id. on purpose?

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 295 at r1 (raw file):

// wrapper for Fetcher.Fetch such that errors in goroutines fail the test
func callWrapper(ctx context.Context, f hpsegreq.Fetcher, req *path_mgmt.HPSegReq,

I'm confused why is that function needed?

Reviewable status: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 33 at r1 (raw file):

Done.

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 108 at r1 (raw file):

Yes, the call returns a list of ids and all the segments for those ids but not the mapping between them.
In the end, I need to reply with HPSegRecs which is defined as follows:

type HPSegRecs struct {
	GroupId *HPGroupId
	Recs    []*seg.Meta
	Err     string
}

So I need to know what id yields which segments.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 72 at r1 (raw file):

Done.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 295 at r1 (raw file):

The problem is that gomock doesn't play nicely with goroutines. If an expectation is violated the gomock controller calls T.FatalF (see here) This in turn terminates the current goroutine. (see here) If that routine is not the main routine the test does not fail and is left running in a broken state.

E.g assume Fetcher tries to resolve an ID locally instead of remotely. This is an error we would want to catch. The mock DB which doesn't expect a call fails and terminates the goroutine. The error doesn't propagate to the main routine, though. The test keeps running and eventually times out because we are missing a reply from the DB. Then it's somewhat unintuitive to extrapolate from the timeout to the actual error.

This wrapper returns failed mock expectations from inside goroutines as errors.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @chaehni)

a discussion (no related file):
Please also add some docstring to GroupInfos field. I took me quite some time to figure out again what the LocalRegistry field was again.

go/hidden_path_srv/internal/hpsegreq/fetcher.go, line 108 at r1 (raw file):

Ok let's leave it as is for the moment.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 295 at r1 (raw file):

I see we have xtest.PanickingReporter for this. Use it like this: ctrl := gomock.NewController(&xtest.PanickingReporter{T: t}) then you shouldn't need that.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 122 at r2 (raw file):

		proto.PathSegType_up,
	)
	// core seg type

I'm a bit confused why do we have a non-hidden segment in the hidden path server? Wouldn't a more "realistic" test just use 3 different down segments towards the same destination?

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 338 at r2 (raw file):

			defer ctrl.Finish()
			groupInfo := &hpsegreq.GroupInfo{
				LocalRegistry: ia110,

If 110 is always a localregistry doesn't it need to be listed in all the groups as registry?

Reviewable status: 5 of 11 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

a discussion (no related file):

Done.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 295 at r1 (raw file):

Done.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 122 at r2 (raw file):

You're right, the segments used in the tests didn't make sense in a HP scenario.
I changed it to something more sensible.

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 338 at r2 (raw file):

GroupInfo holds all the information this HPS has about group configurations. LocalRegistry is the IA of this HPS and Groups is a list of groups this HPS knows about. Basically the HPS needs to know its own IA so it can check it against the groups' Registries list to see if it can be resolved locally. Maybe the name is misleading. It doesn't mean that this HPS is a local registry for all the groups it knows. Probably the name is misleading.

Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chaehni)

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 338 at r2 (raw file):

Ah I see. Yeah I think it's slightly misleading

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 26 at r3 (raw file):

// throughout its life cycle
type GroupInfo struct {
	// IA of a HPS

Comments should always start with the variable name, i.e. LocalRegistry here.

Reviewable status: 8 of 12 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 338 at r2 (raw file):

Renamed LocalRegistry to LocalIA.

go/hidden_path_srv/internal/hpsegreq/registry_algo.go, line 26 at r3 (raw file):

Done.

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chaehni)

go/hidden_path_srv/internal/hpsegreq/fetcher_test.go, line 108 at r4 (raw file):

	seg110_130_112 = markHidden(t, seg.NewMeta(
		g.Beacon([]common.IFIDType{
			graph.If_110_X_130_A,

Goes kind of through 2 cores.

Reviewed 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved