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

colibri: advance request handling #3859

Merged
merged 6 commits into from
Sep 4, 2020
Merged

Conversation

juagargi
Copy link
Contributor

@juagargi juagargi commented Aug 27, 2020

  • Segment request operations are complete
  • E2E request operations with the exception of the admission one are complete

This change is Reviewable

@juagargi juagargi force-pushed the colibri.store2 branch 2 times, most recently from da33db4 to 12dec14 Compare September 1, 2020 14:45
@juagargi juagargi marked this pull request as ready for review September 1, 2020 14:45
@juagargi juagargi mentioned this pull request Sep 2, 2020
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

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


go/cs/reservationstore/store.go, line 149 at r1 (raw file):

		return nil, serrors.WrapStr("error validating request", err, "id", req.ID)
	}
	response, err := s.prepareFailureSegmentResp(&req.Request)

more of a style comment, so feel free to ignore, since it is really subjective.

I would introduce a new line before this line, and one before s.db.BeginTransaction to group according to logical coherence.


go/cs/reservationstore/store.go, line 180 at r1 (raw file):

			"id", req.ID)
	}
	var msg base.MessageWithPath

Just do:

	if req.IsLastAS() {
		return &segment.ResponseIndexConfirmationSuccess{
			Response: *morphSegmentResponseToSuccess(response),
		}, nil
	}
	return req, nil

same comment for all other methods where this applies.

Copy link
Contributor Author

@juagargi juagargi 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, 1 unresolved discussion (waiting on @oncilla)


go/cs/reservationstore/store.go, line 149 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

more of a style comment, so feel free to ignore, since it is really subjective.

I would introduce a new line before this line, and one before s.db.BeginTransaction to group according to logical coherence.

Done.


go/cs/reservationstore/store.go, line 180 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

Just do:

	if req.IsLastAS() {
		return &segment.ResponseIndexConfirmationSuccess{
			Response: *morphSegmentResponseToSuccess(response),
		}, nil
	}
	return req, nil

same comment for all other methods where this applies.

Done.

Copy link
Contributor

@oncilla oncilla 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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @juagargi and @oncilla)


go/cs/reservationstore/store.go, line 149 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

It applies to the other methods as well :)


go/cs/reservationstore/store.go, line 180 at r1 (raw file):

Previously, juagargi (Juan A. Garcia Pardo) wrote…

Done.

As mentioned, this applies to the other methods as well

Copy link
Contributor Author

@juagargi juagargi 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: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @oncilla)


go/cs/reservationstore/store.go, line 149 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

It applies to the other methods as well :)

Done. Done. Too much in a hurry 😛


go/cs/reservationstore/store.go, line 180 at r1 (raw file):

Previously, Oncilla (Dominik Roos) wrote…

As mentioned, this applies to the other methods as well

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@juagargi
Copy link
Contributor Author

juagargi commented Sep 3, 2020

/rebase

@juagargi
Copy link
Contributor Author

juagargi commented Sep 4, 2020

/rebase

@oncilla oncilla changed the title COLIBRI: almost finish handling requests cloibri: advance request handling Sep 4, 2020
@oncilla oncilla changed the title cloibri: advance request handling colibri: advance request handling Sep 4, 2020
@oncilla oncilla merged commit b823a59 into scionproto:master Sep 4, 2020
@juagargi juagargi deleted the colibri.store2 branch September 4, 2020 08:15
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