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

host API cleanup #729

Merged
merged 8 commits into from Jan 15, 2015

Conversation

Projects
None yet
3 participants
@warpfork
Copy link
Contributor

warpfork commented Jan 13, 2015

One tiny change suggested by @archseer; and a rename that will make sense as the volumes API comes in next to this.

@warpfork warpfork force-pushed the host-api-cleanup branch from 4e79288 to ae0645d Jan 13, 2015

@@ -252,7 +252,7 @@ func runDaemon(args *docopt.Args) {

// Check if we are the leader so that we can use the cluster functions directly
sampiCluster := sampi.NewCluster(sampi.NewState())
sampiAPI := sampi.NewHTTPAPI(sampiCluster)
sampiAPI := &sampi.HttpAPI{sampiCluster}

This comment has been minimized.

Copy link
@titanous

titanous Jan 13, 2015

Member

Vet will complain about the missing field name

@warpfork warpfork force-pushed the host-api-cleanup branch from ae0645d to aff42ee Jan 13, 2015

@@ -54,11 +54,11 @@ func (h *Host) streamEvents(id string, w http.ResponseWriter) error {
return nil
}

type httpAPI struct {
type hostAPI struct {

This comment has been minimized.

Copy link
@warpfork

warpfork Jan 13, 2015

Author Contributor

Or actually -- maybe jobAPI?

This comment has been minimized.

Copy link
@archseer

archseer Jan 13, 2015

Contributor

Sounds good

@archseer

This comment has been minimized.

Copy link
Contributor

archseer commented Jan 13, 2015

LGTM, pending that rename

@archseer

This comment has been minimized.

Copy link
Contributor

archseer commented Jan 13, 2015

We could also get rid of sampi.NewCluster. This will need to be changed then:

sampiCluster := sampi.NewCluster(sampi.NewState())

@archseer

This comment has been minimized.

Copy link
Contributor

archseer commented Jan 13, 2015

Ah actually, I'd change sampi.NewCluster to be without arguments and create a new state object within the constructor.

@warpfork warpfork force-pushed the host-api-cleanup branch from 232cab7 to 2600e5d Jan 13, 2015

var jobStream stream.Stream
sh.BeforeExit(func() {
if jobStream != nil {
jobStream.Close()

This comment has been minimized.

Copy link
@titanous

titanous Jan 14, 2015

Member

This seems like unnecessary cleanup?

This comment has been minimized.

Copy link
@archseer

archseer Jan 14, 2015

Contributor

As I said in the commit, I found it easier to follow with the jobStream logic being all in one section.

This comment has been minimized.

Copy link
@titanous

titanous Jan 14, 2015

Member

I mean, closing the stream on exit doesn't really do much, the fds will be reaped by the kernel at exit anyway.

@@ -76,8 +76,8 @@ func (s *Cluster) StreamHostEvents(ch chan host.HostEvent, done chan bool) error
return nil
}

type httpAPI struct {
cluster *Cluster
type HttpAPI struct {

This comment has been minimized.

Copy link
@titanous

titanous Jan 14, 2015

Member

This should be HTTPAPI

warpfork and others added some commits Jan 13, 2015

host: remove unnecessary constructor and param.
Also, rename hostAPI to jobAPI (this seems to accurately describe all functions in the current group, and makes way for more features we'll be adding to the host shortly).

Signed-off-by: Eric Myhre <eric@flynn.io>
host/sampi: remove constructor, export fields.
Signed-off-by: Eric Myhre <eric@flynn.io>
host: Simplify creating the host object.
Since the manifest format has changed, h.Metadata will now always be nil (we're
creating a new blank host object after all). ID can also be directly set, this
was different in the past as we did some pointer trickery.

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

@warpfork warpfork force-pushed the host-api-cleanup branch 2 times, most recently from 1ea1b16 to be89227 Jan 14, 2015

host: Drop unnecessary shutdown cleanup.
This will be dropped on the floor by the OS when the process terminates anyway; no special action is required.

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

@warpfork warpfork force-pushed the host-api-cleanup branch from be89227 to 847273a Jan 14, 2015

@archseer archseer force-pushed the host-api-cleanup branch from 6c6e265 to de64a20 Jan 14, 2015

@titanous

This comment has been minimized.

Copy link
Member

titanous commented Jan 15, 2015

LGTM, though a few commit messages need a pkg/ prefix.

archseer added some commits Jan 14, 2015

pkg/httpclient: Simplify client streams.
Since stream objects are now generic, we can make a more generic client method
for them. Note that since the method doesn't provide a way to specify headers,
StreamJobEvents on the controller cannot be simplified (yet).

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>
pkg/httpclient: Re-introduce DialFunc.
This gets rid of more references to rpcplus; leaving discoverd the last
component to still reference it.

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>
pkg/cluster: Host client no longer needs a close method.
Since we're no longer opening a permanently open socket to the host, there is no
cleanup needed. The current method just called httpclient's Close() method,
which will no-op (it only closes the dialer if dialClose is set, which is nil in
this particular client).

Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>
controller/client: Get rid of unused addr parameter.
Signed-off-by: Blaž Hrastnik <blaz.hrast@gmail.com>

@archseer archseer force-pushed the host-api-cleanup branch from 7063e0a to 1883e60 Jan 15, 2015

archseer added a commit that referenced this pull request Jan 15, 2015

@archseer archseer merged commit b729e3c into master Jan 15, 2015

0 of 2 checks passed

continuous-integration/flynn The Flynn CI build is in progress
Details
continuous-integration/travis-ci The Travis CI build is in progress
Details

@archseer archseer deleted the host-api-cleanup branch Jan 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.