Initial main #1

dgromov · 2017-06-05T02:03:38Z

No description provided.

coveralls · 2017-06-05T02:06:13Z

Changes Unknown when pulling 2265b1c on initial_main into ** on master**.

prateek · 2017-06-05T14:15:29Z

.travis.yml

@@ -10,5 +10,4 @@ dist: trusty
 script:
 - make lint


Follow the convention described here for the makefile.

use make all in the .travis.yml like other M3DB repos: https://github.com/m3db/m3db/blob/master/.travis.yml

prateek · 2017-06-05T14:18:10Z

handler/common.go

+		URI:              r.RequestURI,
+		MethodName:       r.Method,
+		SupportedMethods: supported}
+	body, _ := json.Marshal(response)


nit: handle the error - propagate up/panic

prateek · 2017-06-05T14:19:07Z

handler/common_test.go

+	rawResult := make([]byte, rr.Body.Len())
+	_, err := rr.Body.Read(rawResult)
+	if err != nil {
+		t.Errorf("Encountered error parsing reponse")


shorter:

require.NoError(t, err, "Encountered error parsing response)

prateek · 2017-06-05T14:20:20Z

handler/health/health.go

+	if status != ok {
+		w.WriteHeader(http.StatusInternalServerError)
+	}
+	body, _ := json.Marshal(h)


nit: handle error

Its very unlikely that this will fail. Also, what do you suggest I do in this case?

I agree with Prateek here, all errors should ideally be handled (whether thats performing some action, bubbling them up, panicking, etc.). In this case I think we can log the error and return a 500.

prateek · 2017-06-05T14:22:36Z

handler/health/health.go

+func gatherHostInfo() string {
+	host, err := os.Hostname()
+	if err != nil {
+		host = "unknown"


nit: why not propagate the error up to the m3ctlHealthCheck?

Use a const instead of a magic string here

I took this out of go-common. Its not really an issue, if it is unknown its fine.

prateek · 2017-06-05T14:23:08Z

handler/health/health.go

+)
+
+var (
+	healthURL = "/health"


any reason to make this a var instead of a const?

Just following the pattern we have everywhere else

prateek · 2017-06-05T14:23:39Z

handler/health/health.go

+	"time"
+
+	"github.com/m3db/m3ctl/handler"
+	"github.com/m3db/m3x/instrument"


nit: blank line between repo local and third-party imports

prateek · 2017-06-05T14:24:17Z

handler/rules/rules.go

+	"github.com/m3db/m3x/instrument"
+)
+
+var (


nit: can these be const?

prateek · 2017-06-05T14:24:34Z

handler/rules/rules.go

+	"net/http"
+
+	"github.com/m3db/m3ctl/handler"
+	"github.com/m3db/m3x/instrument"


nit: empty line between these imports

prateek · 2017-06-05T14:26:10Z

handler/rules/global.go

+
+package rules
+
+import "net/http"


nit: import block

prateek · 2017-06-05T14:28:14Z

server/options.go

+}
+
+// NewOptions creates a new set of server options
+func NewOptions() Options {


nit: reorder so the ctor is not in the middle of getters/setters

prateek · 2017-06-05T14:31:33Z

services/m3ctl/main/main.go

+	"github.com/m3db/m3x/log"
+)
+
+type serverConfig struct {


Feels weird to have the struct be internal but the NewHTTPServerOptions method be public. Either export both, or neither.

If you're going to export the config struct, move to another package so other mains can import it

prateek · 2017-06-05T14:31:45Z

services/m3ctl/main/main.go

+
+	etcdclient "github.com/m3db/m3cluster/client/etcd"
+	"github.com/m3db/m3ctl/server"
+	"github.com/m3db/m3ctl/services/m3ctl/serve"


nit: import ordering and spacing

prateek · 2017-06-05T14:36:47Z

services/m3ctl/main/main.go

+}
+
+type kvClientConfiguration struct {
+	Etcd *etcdclient.Configuration `yaml:"etcd"`


any reason to define this instead of not using etcdclient.Configuration directly?

to not pollute the top level config namespace

prateek · 2017-06-05T15:38:36Z

server/server.go

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package server


Can you re-use the newly added m3x/server package?

Will look into this.

Or perhaps the Server struct defined in the http package since this will be an HTTP server: https://golang.org/pkg/net/http/#Server

prateek · 2017-06-05T15:38:57Z

server/handlers.go

+	"github.com/m3db/m3ctl/handler"
+	"github.com/m3db/m3ctl/handler/health"
+	"github.com/m3db/m3ctl/handler/rules"
+	"github.com/m3db/m3x/instrument"


nit: space above this line

coveralls · 2017-06-05T19:47:14Z

Changes Unknown when pulling 143f9e8 on initial_main into ** on master**.

jeromefroe · 2017-06-05T19:52:26Z

glide.yaml

@@ -0,0 +1,11 @@
+package: github.com/m3db/m3ctl
+
+import:


Maybe copy the packages (and their corresponding versions) that are in glide.lock but aren't in glide.yaml into glide.yaml so they don't change when someone runs glide update?

Only for direct dependencies, not transitive ones, yeah?

jeromefroe · 2017-06-05T19:54:00Z

glide.yaml

+  version: 18bc01a7057d55b78e528236e0e9db297bcd4fa3
+- package: github.com/m3db/m3x
+  version: bd6be18ffb184691512eb442561b16a59c15d797
+- package: github.com/stretchr/testify


nit: could mark this under TestImports. For example:

testImport: - package: github.com/stretchr/testify version: 6fe211e493929a8aac0469b93f28b1d0688a9a3a

coveralls · 2017-06-07T17:42:05Z

Changes Unknown when pulling 8caad6a7e144df028377e35641f82f377169a396 on initial_main into ** on master**.

prateek · 2017-06-07T18:27:43Z

glide.yaml

+- package: github.com/uber-go/atomic
+  version: e682c1008ac17bf26d2e4b5ad6cdd08520ed0b22
+- package: github.com/uber-go/tally
+  version: 4b9d9de43ffcb0b7efe67dab2a92c2d58dbf16ab


Tally uses semantic versioning, change this to <4.0.0

prateek · 2017-06-07T18:29:12Z

handler/health/health.go

+	"github.com/m3db/m3ctl/handler"
+	"github.com/m3db/m3x/instrument"
+
+	"fmt"


move this to be along side the other stdlib imports

prateek · 2017-06-07T18:29:39Z

handler/health/health.go

+	unknownName = "unkown"
+)
+
+var ()


prateek · 2017-06-07T18:32:03Z

handler/rules/rules.go

+	"net/http"
+
+	"github.com/gorilla/mux"
+	"github.com/m3db/m3ctl/handler"


separate repo local import

prateek · 2017-06-07T18:32:21Z

server/handlers.go

+import (
+	"github.com/m3db/m3ctl/handler/health"
+	"github.com/m3db/m3ctl/handler/rules"
+	"github.com/m3db/m3x/instrument"


separate repo local v third-party imports

prateek · 2017-06-07T18:32:44Z

services/m3ctl/main/main.go

+	"github.com/m3db/m3x/instrument"
+	"github.com/m3db/m3x/log"
+
+	"github.com/m3db/m3ctl/server"


move repo local imports above third party ones

coveralls · 2017-06-07T19:04:02Z

Changes Unknown when pulling b07947f4f3af13c2b4e62e91542671bb5b44f25e on initial_main into ** on master**.

coveralls · 2017-06-07T19:07:30Z

Changes Unknown when pulling c5b696d2b0b2b523402b67f9ffca5aea6cbb26ff on initial_main into ** on master**.

prateek · 2017-06-07T19:43:15Z

glide.yaml

+- package: github.com/pmezard/go-difflib
+  version: d8ed2627bdf02c080bf22230dbb337003b7aba2d
+- package: github.com/davecgh/go-spew
+  version: 5215b55f46b2b919f50a1df0eaa5886afe4e3b3d


nit: insert newline at the end

prateek · 2017-06-07T19:49:00Z

handler/health/health.go

+)
+
+const (
+	ok          = "OK"


maybe make this a type to make it explicit -

type HealthStatus string const ( Ok HealthStatus = "OK" Fail = "FAIL" )

prateek · 2017-06-07T19:49:48Z

handler/health/health.go

+	unknownName = "unkown"
+)
+
+type healthCheckResult struct {


Does it make sense for this to be exported so other packages can re-use?

Potentially but outside of the scope of this commit. It should go into m3x at that point though.

prateek · 2017-06-07T19:50:18Z

handler/health/health.go

+	return &controller{iOpts: iOpts}
+}
+
+func healthCheck() string {


Why define this function instead of directly using the variable?

This is the healthCheck in the skeleton implementation. It will be more interesting in the future.

coveralls · 2017-06-07T20:07:05Z

Changes Unknown when pulling 58ee895822af306cb1b041bc3641251757ea5891 on initial_main into ** on master**.

coveralls · 2017-06-07T20:07:42Z

Changes Unknown when pulling 58ee895822af306cb1b041bc3641251757ea5891 on initial_main into ** on master**.

coveralls · 2017-06-07T20:14:46Z

Changes Unknown when pulling d08a519 on initial_main into ** on master**.

Dmitriy Gromov added 5 commits June 4, 2017 18:05

It's alive

80d58c0

Adding skeleton for rules functions

3846759

Better Messages and making health into a controller

d2dd03c

Adding a test

bbb2e3a

Fixing tests

2265b1c

dgromov requested review from cw9 and xichen2020 June 5, 2017 02:03

dgromov requested review from prateek and jeromefroe June 5, 2017 14:02

prateek reviewed Jun 5, 2017

View reviewed changes

handler/rules/rules.go Outdated

"github.com/m3db/m3x/instrument"

)

var (

Copy link

prateek Jun 5, 2017

Choose a reason for hiding this comment

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

nit: can these be const?

prateek reviewed Jun 5, 2017

View reviewed changes

handler/rules/rules.go

"net/http"

"github.com/m3db/m3ctl/handler"

"github.com/m3db/m3x/instrument"

Copy link

prateek Jun 5, 2017

Choose a reason for hiding this comment

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

nit: empty line between these imports

prateek reviewed Jun 5, 2017

View reviewed changes

handler/rules/global.go Outdated

package rules

import "net/http"

Copy link

prateek Jun 5, 2017

Choose a reason for hiding this comment

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

nit: import block

prateek reviewed Jun 5, 2017

View reviewed changes

Addressing comments

143f9e8

jeromefroe reviewed Jun 5, 2017

View reviewed changes

Addressing more comments and now using http.Server

3c71666

dgromov force-pushed the initial_main branch 2 times, most recently from 8b40fbc to 8caad6a Compare June 7, 2017 17:40

prateek reviewed Jun 7, 2017

View reviewed changes

handler/health/health.go Outdated

unknownName = "unkown"

)

var ()

Copy link

prateek Jun 7, 2017

Choose a reason for hiding this comment

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

delete

prateek reviewed Jun 7, 2017

View reviewed changes

handler/rules/rules.go Outdated

"net/http"

"github.com/gorilla/mux"

"github.com/m3db/m3ctl/handler"

Copy link

prateek Jun 7, 2017

Choose a reason for hiding this comment

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

separate repo local import

prateek reviewed Jun 7, 2017

View reviewed changes

dgromov force-pushed the initial_main branch from 8caad6a to b07947f Compare June 7, 2017 19:01

dgromov force-pushed the initial_main branch from b07947f to c5b696d Compare June 7, 2017 19:04

prateek reviewed Jun 7, 2017

View reviewed changes

dgromov force-pushed the initial_main branch 2 times, most recently from aa3feec to 58ee895 Compare June 7, 2017 20:05

Now using gorilla mux

d08a519

dgromov force-pushed the initial_main branch from 58ee895 to d08a519 Compare June 7, 2017 20:11

dgromov merged commit 3d0f679 into master Jun 7, 2017

dgromov deleted the initial_main branch June 7, 2017 20:18

dgromov restored the initial_main branch June 7, 2017 20:19

dgromov mentioned this pull request Jun 7, 2017

Revert "Initial main" #2

Merged

dgromov deleted the initial_main branch June 7, 2017 21:50

Initial main #1

Initial main #1

Conversation

dgromov commented Jun 5, 2017

coveralls commented Jun 5, 2017

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 5, 2017

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

coveralls commented Jun 7, 2017

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

Choose a reason for hiding this comment

coveralls commented Jun 7, 2017

coveralls commented Jun 7, 2017

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

Choose a reason for hiding this comment

coveralls commented Jun 7, 2017

coveralls commented Jun 7, 2017

coveralls commented Jun 7, 2017