Remove layers of indirection between apiinstaller and resthandler #4263

smarterclayton · 2015-02-09T23:25:03Z

Make the RESTHandler feel more go-restful, set the stage for adding
new types of subresource collections.

Extracted from #4248 (which is about status)

mbforbes · 2015-02-10T00:04:29Z

@smarterclayton thoughts on who might be good to review?

smarterclayton · 2015-02-10T00:23:07Z

Nikhil or anyone who wants to get more involved in the api server, @davidopp maybe

On Feb 9, 2015, at 7:04 PM, Maxwell Forbes notifications@github.com wrote:

@smarterclayton thoughts on who might be good to review?

—
Reply to this email directly or view it on GitHub.

mbforbes · 2015-02-10T00:33:14Z

Nikhil for now, feel free to reassign---thanks!

smarterclayton · 2015-02-10T14:15:38Z

I've got another commit coming to this pull to remove <-chan apiserver.RESTResult. Will make it so that we can reuse the REST storage in many contexts without having to have async behavior. In the future, if we want to do cancellation we'll be doing it through the api.Context stuff.

smarterclayton · 2015-02-10T14:24:21Z

@bgrant0607 the combination of this change (and the proposed) will dramatically simplify the generic code path - in my other pull I have pods mostly refactored, and the amount of custom code is dropping fast.

bgrant0607 · 2015-02-10T16:39:33Z

Awesome. Thanks. Will take a look, though I'm in meetings all day, so don't block on me.

smarterclayton · 2015-02-10T16:48:46Z

Rest will look a lot like the client interface again.

On Feb 10, 2015, at 11:39 AM, Brian Grant notifications@github.com wrote:

Awesome. Thanks. Will take a look, though I'm in meetings all day, so don't block on me.

—
Reply to this email directly or view it on GitHub.

bgrant0607 · 2015-02-10T16:58:45Z

pkg/apiserver/apiserver_test.go

@@ -961,7 +1068,7 @@ func TestCreateTimeout(t *testing.T) {

 	simple := &Simple{Other: "foo"}
 	data, _ := codec.Encode(simple)
-	itemOut := expectApiStatus(t, "POST", server.URL+"/prefix/version/foo?timeout=4ms", data, http.StatusAccepted)
+	itemOut := expectApiStatus(t, "POST", server.URL+"/prefix/version/foo?timeout=4ms", data, 504)


Could you please define a constant for 504 here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/api/errors/errors.go#L30

bgrant0607 · 2015-02-10T17:02:17Z

Lots of good stuff here. LGTM.

@nikhiljindal Any comments?

smarterclayton · 2015-02-10T18:03:58Z

Ok, this is ready for review (running e2e now). This completely removes the RESTStorage interfaces from having to know about asynchronous behavior, which means those APIs now look like client apis, which means that we can much more cleanly substitute registry for client APIs again. This sets the stage for generic registries which can now service RESTStorage requests directly without having to wrap async behavior.

nikhiljindal · 2015-02-10T18:42:33Z

I am out of office till Thursday.
Please do not wait for me.
On Feb 10, 2015 10:04 AM, "Clayton Coleman" notifications@github.com
wrote:

Ok, this is ready for review (running e2e now). This completely removes
the RESTStorage interfaces from having to know about asynchronous behavior,
which means those APIs now look like client apis, which means that we can
much more cleanly substitute registry for client APIs again. This sets the
stage for generic registries which can now service RESTStorage requests
directly without having to wrap async behavior.

—
Reply to this email directly or view it on GitHub
#4263 (comment)
.

smarterclayton · 2015-02-10T21:42:06Z

I've got a mostly clean e2e run, trying to figure out whether the e2e one is a flake or not

smarterclayton · 2015-02-10T22:47:21Z

I have seen all of the different tests in e2e pass, just not in one run.

smarterclayton · 2015-02-11T21:21:36Z

@brendandburns if you have time to review I would appreciate it - this cleanup is important for getting to a single etcd / registry / rest implementation.

Make the RESTHandler feel more go-restful, set the stage for adding new types of subresource collections.

Also make sure all POST operations return 201 by default. Removes the remainder of the asych logic in RESTStorage and leaves it up to the API server to expose that behavior.

bgrant0607 · 2015-02-12T00:21:33Z

Thanks @smarterclayton. Looking at this now.

bgrant0607 · 2015-02-12T00:34:38Z

pkg/apiserver/api_installer.go

-		canonicalPrefix:        a.restHandler.canonicalPrefix,
-		selfLinker:             a.restHandler.selfLinker,
-		apiRequestInfoResolver: a.restHandler.apiRequestInfoResolver,
+		storage: a.group.storage,


Why doesn't it make sense to just pass a.group, as opposed to the individual fields? Because they're all private, currently?

Yeah, I think I'd like to refactor watch and redirect like the other methods, at which point group can be refactored out as well.

On Feb 11, 2015, at 7:34 PM, Brian Grant notifications@github.com wrote:

In pkg/apiserver/api_installer.go:

@@ -49,16 +53,16 @@ func (a *APIInstaller) Install() (ws *restful.WebService, errors []error) {

// Initialize the custom handlers. watchHandler := (&WatchHandler{

storage: a.restHandler.storage,

codec: a.restHandler.codec,

canonicalPrefix: a.restHandler.canonicalPrefix,

selfLinker: a.restHandler.selfLinker,

apiRequestInfoResolver: a.restHandler.apiRequestInfoResolver,

storage: a.group.storage,
Why doesn't it make sense to just pass a.group, as opposed to the individual fields? Because they're all private, currently?

—
Reply to this email directly or view it on GitHub.

nikhiljindal · 2015-02-12T06:26:09Z

Lgtm thanks!

bgrant0607 · 2015-02-12T10:15:02Z

pkg/apiserver/api_installer.go

@@ -148,6 +171,19 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage

 		itemPath := path + "/{name}"
 		nameParams := append(params, nameParam)
+		namespaceFn = func(req *restful.Request) (namespace string, err error) {
+			return


I'm still a Go newbie, I guess. What does this do? I don't see a definition of namespace.

Its a named return parameter, which have zero values to start.

However, this made me realize that empty names and namespaces (for namespaced scopes) must be rejected with an error. Will fix that.

On Feb 12, 2015, at 5:15 AM, Brian Grant notifications@github.com wrote:

In pkg/apiserver/api_installer.go:

@@ -148,6 +171,19 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage RESTStorage

itemPath := path + "/{name}" nameParams := append(params, nameParam)

namespaceFn = func(req *restful.Request) (namespace string, err error) {

return
I'm still a Go newbie, I guess. What does this do? I don't see a definition of namespace.

—
Reply to this email directly or view it on GitHub.

Yuck. Naked returns in the middle of a large function are incredibly hard to read. See https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Named_Result_Parameters

Naked returns are okay if the function is a handful of lines.

I think these functions can be moved to static functions, they don't need to be closures. Will do that in a follow up.

----- Original Message -----

@@ -148,6 +171,19 @@ func (a *APIInstaller) registerResourceHandlers(path
string, storage RESTStorage

itemPath := path + "/{name}" nameParams := append(params, nameParam)

namespaceFn = func(req *restful.Request) (namespace string, err error) {

return

Yuck. Naked returns in the middle of a large function are incredibly hard to
read. See
https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Named_Result_Parameters

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/4263/files#r24597352

Right. It's difficult to tell at first glance that most (or all?) of the naked returns are inside tiny functions here.

bgrant0607 · 2015-02-12T10:28:11Z

LGTM, also.
Note: This fixes #2031.

smarterclayton · 2015-02-12T14:46:09Z

Pushed one more commit to clearly return an error if name is empty.

Remove layers of indirection between apiinstaller and resthandler

smarterclayton · 2015-02-12T17:25:33Z

@nikhiljindal opened #4387 to handle rename of "TryAgainLater" to "ServerTimeout" (as distinct from RequestTimeout and GatewayTimeout)

smarterclayton · 2015-02-16T01:02:02Z

This broke cross namespace queries (for which we apparently have zero test cases). Fix incoming...

smarterclayton mentioned this pull request Feb 9, 2015

Create a new unified storage object for Pods #4248

Merged

smarterclayton force-pushed the simplify_apiserver branch 2 times, most recently from 0bc79c9 to 56dd906 Compare February 9, 2015 23:35

mbforbes assigned nikhiljindal Feb 10, 2015

smarterclayton force-pushed the simplify_apiserver branch from 56dd906 to 60f596b Compare February 10, 2015 14:02

bgrant0607 reviewed Feb 10, 2015
View reviewed changes

smarterclayton force-pushed the simplify_apiserver branch from 60f596b to 8fe356e Compare February 10, 2015 17:59

smarterclayton force-pushed the simplify_apiserver branch 3 times, most recently from 587ba38 to 69397eb Compare February 10, 2015 21:26

smarterclayton force-pushed the simplify_apiserver branch 2 times, most recently from 92741b6 to eb15a81 Compare February 10, 2015 22:21

smarterclayton closed this Feb 11, 2015

smarterclayton deleted the simplify_apiserver branch February 11, 2015 02:21

smarterclayton restored the simplify_apiserver branch February 11, 2015 21:20

smarterclayton reopened this Feb 11, 2015

Remove layers of indirection between apiinstaller and resthandler

d167c11

Make the RESTHandler feel more go-restful, set the stage for adding new types of subresource collections.

smarterclayton added 2 commits February 11, 2015 16:26

Remove asynchronous channel on RESTStorage interfaces

79cb930

RESTStorage should not need to know about async behavior

26f08b7

Also make sure all POST operations return 201 by default. Removes the remainder of the asych logic in RESTStorage and leaves it up to the API server to expose that behavior.

smarterclayton force-pushed the simplify_apiserver branch from eb15a81 to 26f08b7 Compare February 11, 2015 21:26

Add monitoring as a filter

8f6e360

roberthbailey assigned brendandburns and unassigned nikhiljindal Feb 11, 2015

bgrant0607 reviewed Feb 12, 2015
View reviewed changes

smarterclayton mentioned this pull request Feb 12, 2015

WIP - Add trace header for all incoming requests #3826

Closed

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2015

bgrant0607 reviewed Feb 12, 2015
View reviewed changes

Return an error when names are empty

e6fdac1

smarterclayton added a commit that referenced this pull request Feb 12, 2015

Merge pull request #4263 from smarterclayton/simplify_apiserver

0a2d713

Remove layers of indirection between apiinstaller and resthandler

smarterclayton merged commit 0a2d713 into kubernetes:master Feb 12, 2015

smarterclayton mentioned this pull request Feb 12, 2015

Rename TryAgainLater status code to ServerTimeout #4387

Merged

smarterclayton mentioned this pull request Feb 12, 2015

Fix cross-namespace LIST and WATCH #4395

Merged

bgrant0607 mentioned this pull request Feb 13, 2015

Return 201 when objects are created #2031

Closed

smarterclayton unassigned brendandburns Aug 12, 2015

wallrj mentioned this pull request Aug 5, 2016

List nodes ClusterHQ/flocker-go#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove layers of indirection between apiinstaller and resthandler #4263

Remove layers of indirection between apiinstaller and resthandler #4263

smarterclayton commented Feb 9, 2015

mbforbes commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

mbforbes commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

bgrant0607 commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

bgrant0607 Feb 10, 2015

bgrant0607 commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

nikhiljindal commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 11, 2015

bgrant0607 commented Feb 12, 2015

bgrant0607 Feb 12, 2015

smarterclayton Feb 12, 2015

nikhiljindal commented Feb 12, 2015

bgrant0607 Feb 12, 2015

smarterclayton Feb 12, 2015

roberthbailey Feb 12, 2015

smarterclayton Feb 12, 2015

roberthbailey Feb 12, 2015

bgrant0607 commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

smarterclayton commented Feb 16, 2015

Remove layers of indirection between apiinstaller and resthandler #4263

Remove layers of indirection between apiinstaller and resthandler #4263

Conversation

smarterclayton commented Feb 9, 2015

mbforbes commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

mbforbes commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

bgrant0607 commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

Choose a reason for hiding this comment

bgrant0607 commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

nikhiljindal commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 10, 2015

smarterclayton commented Feb 11, 2015

bgrant0607 commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

smarterclayton commented Feb 12, 2015

smarterclayton commented Feb 16, 2015