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

MYST-291 Karakiri endpoint #139

Merged
merged 19 commits into from Feb 7, 2018

Conversation

Projects
None yet
4 participants
@donce
Copy link
Contributor

commented Feb 2, 2018

No description provided.

@donce donce requested review from shroomist, ignasbernotas, tadovas, Waldz and zolia Feb 2, 2018

@@ -2,6 +2,7 @@ package monitor

import (
"errors"
node_cmd "github.com/mysterium/node/cmd"

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

why cmd, wrong package name in this scope?

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

Yes - all command methods uses cmd for instance variable. Didn't want to rename all instance variables, because package name cmd is not the best one and we should rename this package instead of trying to avoid using cmd anywhere.

if err := cmdCli.Run(); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
}

cmd.NewTerminator(stop)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

this place can register 2nd sigterm handler together with CLI

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

We can't invoke Kill of cli command, because it blocks.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

I mean 2 sig handlers

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

Inspected that signals handled by us and by Readline do not overlap.

if clientCommand != nil {
killers = append(killers, serverCommand.Kill)
}
stop := cmd.NewApplicationStopper(killers...)

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

Array of callbacks looks overcomplicated

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

Simplified.


func newStopHandler(stop Stopper) httprouter.Handle {
return func(_ http.ResponseWriter, _ *http.Request, _ httprouter.Params) {
stop()

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

Strange that response flushing and delaying logic is not here, but in factory

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

Moved to endpoint.

type Stopper func()

// AddRouteForStop adds stop route to given router
func AddRouteForStop(router *httprouter.Router, stop Stopper) {

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

We are in very deep layer, what king of stopper is it?
Too generic name is it program stopper?

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

king of stopper?

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

maybe ApplicationStopper

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

done

)

// Stopper stops application and performs required cleanup tasks
type Stopper func()

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Member

Is duplicated interface Stopper needed in both packages?

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

Yes - interfaces are defined on receiver side and they are very lightweight, server the purpose of labeling what these functions expect.

killers = append(killers, cmdRun.Kill)
}
// TODO: add CLI killer once it's not blocking anymore
cmd.NewApplicationStopper(killers...)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 5, 2018

Member

NewApplicationStopper creates func which is not used anywhere ?

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

Fixed.


func newStopHandler(stop Stopper) httprouter.Handle {
return func(_ http.ResponseWriter, _ *http.Request, _ httprouter.Params) {
stop()

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 5, 2018

Member

post with no response body should report http status accepted (202) to indicate that action will be applied. Let's try to follow rest principles.

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

Done.

os.Exit(0)
}

cmdRun = client.NewCommand(options, stop)

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 5, 2018

Member

cmdRun creator needs stop function which references cmdRun itself, that looks complicated :(

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor

Yes indeed - any suggestions?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 5, 2018

Member

That's why sad face

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 5, 2018

Member

... Or we can try idea we talked about recently.

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

This was simplified.

This comment has been minimized.

Copy link
@tadovas

type exitter func(code int)

func newStopper(exit exitter, killers ...Killer) func() {

This comment has been minimized.

Copy link
@shroomist

shroomist Feb 5, 2018

Contributor

to me it's confusing as there are three terms introduced in this PR -- Killer Stopper and Exiter. Thats in addition to an existing Terminator.
I just really want to understand heuristic differences of these terms. thanks.

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Author Contributor
  • Killer is a callback which tries to kill a command, but can fail (can return error).
  • Stopper is a callback which stops the application by invoking multiple killers and exiting application using exiter.
  • exiter is just a local interface (that's why it's private) for dependency-injecting os.exit to be able to test stopper.

This comment has been minimized.

Copy link
@shroomist

shroomist Feb 5, 2018

Contributor

thanks, if I understand correctly, Stopper == makeUnstoppable(Killer)
Stopper in this case takes the role of ultimate Destroyer -- it destroys the app no matter what.
and exiter is the tool that he uses to destroy. bazooka? xD

This comment has been minimized.

Copy link
@donce

donce Feb 7, 2018

Author Contributor

exaclty 👍

@tadovas

tadovas approved these changes Feb 7, 2018

Copy link
Member

left a comment

LGTM

os.Exit(0)
}

cmdRun = client.NewCommand(options, stop)

This comment has been minimized.

Copy link
@tadovas

@donce donce merged commit 0bcf788 into master Feb 7, 2018

@donce donce deleted the feature/MYST-291-stop-endpoint branch Feb 7, 2018

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.