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

PS: implement request deduplication #2017

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Oct 23, 2018

Fixes #1858


This change is Reviewable

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @kormat)


go/path_srv/internal/handlers/psdedupe.go, line 42 at r1 (raw file):

}

type psDeduper struct {

Maybe rename this to something like segReqRoundTripper? It's not actually a deduper, as the deduplication happens before its method is called, in the wrapper built below in NewDeduper.


go/path_srv/internal/handlers/psdedupe.go, line 46 at r1 (raw file):

}

func (pd *psDeduper) segsRequestFunc(ctx context.Context,

If psDeduper is renamed, this can be called something simple like roundTrip.


go/path_srv/internal/handlers/psdedupe.go, line 62 at r1 (raw file):

}

func (h *segReqHandler) getSegsFromNetwork(ctx context.Context,

This should be moved near segReqHandler's other methods (e.g., underneath fetchAndSaveSegs.


go/path_srv/internal/handlers/psdedupe.go, line 78 at r1 (raw file):

		return response.Data.(*path_mgmt.SegReply), nil
	case <-ctx.Done():
		return nil, common.NewBasicError("Context done while waiting for Segs",

This wrap fits on one line.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker 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, 5 unresolved discussions (waiting on @lukedirtwalker and @kormat)


go/path_srv/internal/handlers/psdedupe_test.go, line 48 at r1 (raw file):

		}
		reply := &path_mgmt.SegReply{}
		msger.EXPECT().GetSegs(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(

As discussed with @scrye we should no really work against a "real" deduper. We should change deduper to be an interface and work against a mock here. Then we only have to check the public API: 1) Multiple requests should not have a different deduper.
2) A request calls the deduper if it needs to go to the network.

Testing dedupe is already in the dedupe test.
Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker 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: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @scrye, @lukedirtwalker, and @kormat)


go/path_srv/internal/handlers/psdedupe.go, line 42 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Maybe rename this to something like segReqRoundTripper? It's not actually a deduper, as the deduplication happens before its method is called, in the wrapper built below in NewDeduper.

I removed the struct. It wasn't really needed just to bind the messenger.


go/path_srv/internal/handlers/psdedupe.go, line 46 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

If psDeduper is renamed, this can be called something simple like roundTrip.

I inlined this in NewDeduper and call it requestFunc.


go/path_srv/internal/handlers/psdedupe.go, line 62 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This should be moved near segReqHandler's other methods (e.g., underneath fetchAndSaveSegs.

Done.


go/path_srv/internal/handlers/psdedupe.go, line 78 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This wrap fits on one line.

Done.


go/path_srv/internal/handlers/psdedupe_test.go, line 48 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

As discussed with @scrye we should no really work against a "real" deduper. We should change deduper to be an interface and work against a mock here. Then we only have to check the public API: 1) Multiple requests should not have a different deduper.
2) A request calls the deduper if it needs to go to the network.

Doesn't really make sense. The deduper is already properly tested. So we would just test that we are using it.

Copy link
Contributor

@scrye scrye 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 6 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @kormat)

@lukedirtwalker lukedirtwalker merged commit d0ddc0f into scionproto:master Oct 29, 2018
@lukedirtwalker lukedirtwalker deleted the pubDedupePSRequests branch October 29, 2018 12:33
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.

2 participants