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

messenger: Add handler decorator that is resource aware #2555

Merged

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Apr 2, 2019

If a resource is not healthy the handler should not be executed, instead an error should be returned.
This can be used to make handlers resource aware so that they don't execute when e.g. a database is not available.


This change is Reviewable

@lukedirtwalker lukedirtwalker requested a review from scrye April 2, 2019 10:25
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 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)


go/lib/infra/common.go, line 251 at r1 (raw file):

// ResourceHealth indicates the health of a resource. A resource could for example be a database.
// The resource health can be registered on the messenger, if any resource is not healthy it will

Change the "resource health can be registered on the messenger" part to instead mention the handler decorator.


go/lib/infra/common.go, line 332 at r1 (raw file):

	// RegisterResource registers the given resource with the messenger. If the resource is not
	// healthy the messenger replies to all requests with an error.
	RegisterResource(resource ResourceHealth)

This is no longer needed.


go/lib/infra/common_test.go, line 54 at r1 (raw file):

		rHandler := infra.NewResourceAwareHandler(handler, &mockResource{name: "tstFail", healthy: false})
		rwMock := mock_infra.NewMockResponseWriter(ctrl)
		ctx := infra.NewContextWithResponseWriter(context.TODO(), rwMock)

Use context.Background() here instead, it will stay out of code greps that search for unfinished context implementations.

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: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @scrye)


go/lib/infra/common.go, line 251 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Change the "resource health can be registered on the messenger" part to instead mention the handler decorator.

Done.


go/lib/infra/common.go, line 332 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

This is no longer needed.

Done.


go/lib/infra/common_test.go, line 54 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Use context.Background() here instead, it will stay out of code greps that search for unfinished context implementations.

Done.

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 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


go/lib/infra/common.go, line 251 at r2 (raw file):

// ResourceHealth indicates the health of a resource. A resource could for example be a database.
// The resource health can be added to a handler, so that the handler only replies if all it's

since this needs a rebase anyway :P, it's -> its.

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 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukedirtwalker lukedirtwalker changed the title messenger: Make it possible to register resources messenger: Add handler decorator that is resource aware Apr 3, 2019
@lukedirtwalker lukedirtwalker merged commit 4e57ef1 into scionproto:master Apr 3, 2019
@lukedirtwalker lukedirtwalker deleted the pubMessengerReplyErr branch April 3, 2019 10:51
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