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

Simplify SCIOND client API implementation #3244

Merged
merged 2 commits into from
Oct 14, 2019
Merged

Simplify SCIOND client API implementation #3244

merged 2 commits into from
Oct 14, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented Oct 11, 2019

Specifically:

  • remove the SCIOND client API caching layer; it had a low impact on
    performance for the code complexity it added;
  • applications were using the "reconnecting" implementation of the
    client API; now that is the only available implementation;
  • migrate to serrors;
  • remove locking since now calls are independent;

This change is Reviewable

@scrye scrye added SNET refactor Change that focuses around reducing tech debt labels Oct 11, 2019
@scrye scrye requested a review from lukedirtwalker October 11, 2019 14:22
@scrye scrye self-assigned this Oct 11, 2019
Copy link
Collaborator

@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.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scrye)


go/lib/sciond/sciond.go, line 48 at r1 (raw file):

U

usually we start error messages with lower case. https://github.com/golang/go/wiki/CodeReviewComments#error-strings


go/lib/sciond/sciond.go, line 82 at r1 (raw file):

_ bool

why not just remove it already?


go/lib/sciond/sciond.go, line 141 at r1 (raw file):

	if initialCheckTimeout != 0 {
		deadline := time.Now().Add(initialCheckTimeout)
		deadlineCtx, cancelF := context.WithDeadline(context.Background(), deadline)

wait why not just use context.WithTimeout since you get a timeout?


go/lib/sciond/sciond.go, line 148 at r1 (raw file):

	dispatcher, err := c.ctxAwareConnect(ctx)
	if err != nil {
		return serrors.WrapStr("Unable to connect to SCIOND", err)

ditto lower case

Copy link
Contributor Author

@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.

Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @scrye)


go/lib/sciond/sciond.go, line 48 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
U

usually we start error messages with lower case. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Done.


go/lib/sciond/sciond.go, line 82 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
_ bool

why not just remove it already?

Forgot about it. Fixed.


go/lib/sciond/sciond.go, line 141 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

wait why not just use context.WithTimeout since you get a timeout?

Ah, it used to be lower in the function after some of the timeout was used up already, so an absolute deadline was simpler.

Now it no longer makes sense; fixed.


go/lib/sciond/sciond.go, line 148 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

ditto lower case

Done.

Copy link
Collaborator

@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.

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

scrye added 2 commits October 14, 2019 15:20
Specifically:
- remove the SCIOND client API caching layer; it had a low impact on
performance for the code complexity it added;
- applications were using the "reconnecting" implementation of the
client API; now that is the only available implementation;
- migrate to serrors;
- remove locking since now calls are independent;
@scrye scrye merged commit a21f500 into scionproto:master Oct 14, 2019
@scrye scrye deleted the pubpr-simplify-sciond-client-api branch October 14, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Change that focuses around reducing tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants