Refactor grpc server creation in own package #1487

stefanvassilev · 2019-04-23T18:48:32Z

Signed-off-by: stefan vassilev [email protected]

Which problem is this PR solving?

Resolves [query] Refactor HTTP/gRPC server creation out of main() #1482

Short description of the changes

Move creation of grpcserver to grpcserver package

codecov · 2019-04-23T20:19:37Z

Codecov Report

Merging #1487 into master will decrease coverage by 0.02%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
- Coverage   99.81%   99.79%   -0.03%     
==========================================
  Files         181      182       +1     
  Lines        8652     8711      +59     
==========================================
+ Hits         8636     8693      +57     
- Misses          8        9       +1     
- Partials        8        9       +1

Impacted Files	Coverage Δ
cmd/query/app/grpc_handler.go	`97.29% <100%> (ø)`	⬆️
cmd/query/app/server.go	`96.61% <96.61%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76c0164...7d3b922. Read the comment docs.

yurishkuro

good start!

yurishkuro · 2019-04-23T22:11:59Z

cmd/query/app/grpcserver/server.go

+	// Prepare cmux conn.
+	conn, err := net.Listen("tcp", fmt.Sprintf(":%d", g.QueryOptions.Port))
+	if err != nil {
+		g.Logger.Fatal("Could not start listener", zap.Error(err))


as a reusable package, it's better to return errors than to log them (main will log)

yurishkuro · 2019-04-23T22:19:26Z

cmd/query/app/grpcserver/server.go

+	QuerySvc        querysvc.QueryService
+	Logger          *zap.Logger
+	Tracer          opentracing.Tracer
+	QueryOptions    *app.QueryOptions


looks like only Port is used, I would pass the Port explicitly and narrow the API surface

yurishkuro · 2019-04-23T22:20:16Z

cmd/query/app/grpcserver/server.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package grpcserver


this is not, strictly speaking, a grpc server, I would call it just "server". It can go under app/server.go

yurishkuro · 2019-04-23T22:21:44Z

cmd/query/app/grpcserver/server.go

+}
+
+// Build a GrpcServer
+func (g *Builder) Build() *GrpcServer {


what is the value in separating Build() and Start()? Maybe just have a single Server struct and Start() function.

yurishkuro · 2019-04-23T22:22:11Z

cmd/query/app/grpcserver/server.go

+// Start http, GRPC and cmux servers concurrently
+func (s *GrpcServer) Start() {
+
+	logger := s.logger // shortcut


not necessary in new code

yurishkuro · 2019-04-23T22:25:31Z

cmd/query/main.go

@@ -122,58 +117,17 @@ func main() {
 			compressHandler := handlers.CompressHandler(r)
 			recoveryHandler := recoveryhandler.NewRecoveryHandler(logger, true)


looks like L102-118 could also be refactored into httpserver pkg, and reused in all-in-one

yurishkuro · 2019-04-23T22:26:51Z

cmd/query/app/grpcserver/server.go

+
+func (g *Builder) createHandler() *app.GRPCHandler {
+	return app.NewGRPCHandler(g.QuerySvc, g.Logger, g.Tracer)
+}


would be nice to have similar function for HTTP Handler.

yurishkuro · 2019-04-23T22:27:26Z

cmd/query/app/grpcserver/server.go

+	Svc             *flags.Service
+	RecoveryHandler http.Handler
+	QuerySvc        querysvc.QueryService
+	Logger          *zap.Logger


Svc includes logger

yurishkuro · 2019-04-23T22:28:42Z

cmd/query/app/grpcserver/server.go

+// Builder provides the configuration used to build a GrpcServer
+type Builder struct {
+	Svc             *flags.Service
+	RecoveryHandler http.Handler


"recovery" is not relevant here, it's just HTTP handler.

I would prefer symmetry between GRPC and HTTP, i.e. either we accept fully instantiated handlers, OR we create both of them in this class (the latter is better, can share code w/ all-in-one)

yurishkuro · 2019-04-23T22:33:05Z

cmd/query/app/grpcserver/server.go

+		if err := s.httpServer.Serve(s.listeners[httpListener]); err != nil {
+			logger.Fatal("Could not start HTTP server", zap.Error(err))
+		}
+		s.svc.HC().Set(healthcheck.Unavailable)


not a goal of this PR, but we should really revisit this, because it's feasible that one of the servers will fail to start and set HC=not-healthy, while the others would start up fine and main will set HC to OK.

The goroutines should just be signaling to main() that they failed, and main can set the healthcheck.

Maybe add a TODO to fix this separately.

In the docs for logger.Fatal it says that

// .... // The logger then calls os.Exit(1), even if logging at FatalLevel is

wouldn't that mean that anything below the log is dead code and won't be executed? If so should we log in the goroutine with fatal or just have a mechanism for propagation of the errs to main(), where they'll be logged.

cmd/query/app/server.go

Signed-off-by: stefan vassilev <[email protected]>

Resolves racing condition when healthcheck's status is set to available after it is set to unavailable by server initialization process Signed-off-by: stefan vassilev <[email protected]>

cmd/query/main.go

yurishkuro · 2019-04-26T03:46:59Z

cmd/flags/service.go

@@ -49,18 +49,23 @@ type Service struct {
 	MetricsFactory metrics.Factory

 	signalsChannel chan os.Signal
+
+	// HcStatusChannel is used for error propagation
+	HcStatusChannel chan healthcheck.Status


this doesn't need to be exposed publicly, instead you can have a method SetHealthCheckStatus() - assuming this is how we want goroutines to interact with Service

cmd/query/app/server.go

cmd/flags/service.go

yurishkuro · 2019-04-26T03:53:43Z

cmd/query/app/server.go

+}
+
+// NewServer creates and initializes Server
+func NewServer(svc *flags.Service, router *mux.Router, querySvc querysvc.QueryService, tracker opentracing.Tracer, port int) (*Server, error) {


tracker -> tracer

yurishkuro · 2019-04-26T03:55:06Z

cmd/query/app/server.go

+		httpListener: cmuxServer.Match(cmux.Any()),
+		httpServer:   createHTTPServer(router, svc.Logger),
+		muxServer:    cmuxServer,
+		querySvc:     querysvc.QueryService{},


querysvc.QueryService{} ?

my IDE does this sometimes, my bad

yurishkuro · 2019-04-26T03:56:22Z

cmd/query/app/server_test.go

+	err := flagsSvc.Admin.Serve()
+	assert.NoError(t, err)
+	flagsSvc.Logger = zap.NewNop()
+	go flagsSvc.RunAndThen(func() {


what happens here?

Signed-off-by: stefan vassilev <[email protected]>

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2019-04-26T17:51:39Z

build is failing on gosec #1501

yurishkuro · 2019-04-26T18:41:39Z

Thanks for PR!

yurishkuro · 2019-04-26T20:11:42Z

Follow-up: #1503

stefanvassilev requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners April 23, 2019 18:48

stefanvassilev force-pushed the grpc-creation branch 2 times, most recently from 62316bb to 7a06089 Compare April 23, 2019 19:24

stefanvassilev force-pushed the grpc-creation branch from 9678eca to 65d0b2a Compare April 23, 2019 20:21

yurishkuro reviewed Apr 23, 2019

View reviewed changes

stefanvassilev force-pushed the grpc-creation branch 2 times, most recently from a982dcd to 0a8c857 Compare April 24, 2019 16:54

annanay25 reviewed Apr 24, 2019

View reviewed changes

cmd/query/app/server.go Show resolved Hide resolved

stefanvassilev force-pushed the grpc-creation branch from 0a8c857 to 8eda072 Compare April 24, 2019 22:53

stefanvassilev added 5 commits April 26, 2019 03:34

Refactor grpc server creation in own package

8b2b063

Signed-off-by: stefan vassilev <[email protected]>

Address PR comments

33bde0a

Signed-off-by: stefan vassilev <[email protected]>

Add info log before creation of cmux server

e35d901

Signed-off-by: stefan vassilev <[email protected]>

Add test

381a2c8

Signed-off-by: stefan vassilev <[email protected]>

Propagate error to main

bd28b50

Resolves racing condition when healthcheck's status is set to available after it is set to unavailable by server initialization process Signed-off-by: stefan vassilev <[email protected]>

stefanvassilev force-pushed the grpc-creation branch from a316701 to bd28b50 Compare April 26, 2019 01:35

yurishkuro reviewed Apr 26, 2019

View reviewed changes

Address PR comments

00a9e74

Signed-off-by: stefan vassilev <[email protected]>

stefanvassilev force-pushed the grpc-creation branch from a0ddb2c to 00a9e74 Compare April 26, 2019 10:28

Yuri Shkuro added 2 commits April 26, 2019 12:45

Some refactoring

b32e417

Signed-off-by: Yuri Shkuro <[email protected]>

fix lint

b177003

Signed-off-by: Yuri Shkuro <[email protected]>

Merge branch 'master' into grpc-creation

7d3b922

yurishkuro approved these changes Apr 26, 2019

View reviewed changes

yurishkuro merged commit bb786ae into jaegertracing:master Apr 26, 2019

stefanvassilev deleted the grpc-creation branch April 26, 2019 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor grpc server creation in own package #1487

Refactor grpc server creation in own package #1487

stefanvassilev commented Apr 23, 2019

codecov bot commented Apr 23, 2019 •

edited

Loading

yurishkuro left a comment

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

yurishkuro Apr 23, 2019

stefanvassilev Apr 24, 2019

yurishkuro Apr 26, 2019

yurishkuro Apr 26, 2019

yurishkuro Apr 26, 2019

stefanvassilev Apr 26, 2019

yurishkuro Apr 26, 2019

yurishkuro commented Apr 26, 2019

yurishkuro commented Apr 26, 2019

yurishkuro commented Apr 26, 2019

		@@ -122,58 +117,17 @@ func main() {
		compressHandler := handlers.CompressHandler(r)
		recoveryHandler := recoveryhandler.NewRecoveryHandler(logger, true)

Refactor grpc server creation in own package #1487

Refactor grpc server creation in own package #1487

Conversation

stefanvassilev commented Apr 23, 2019

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 23, 2019 • edited Loading

Codecov Report

yurishkuro left a 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

yurishkuro commented Apr 26, 2019

yurishkuro commented Apr 26, 2019

yurishkuro commented Apr 26, 2019

codecov bot commented Apr 23, 2019 •

edited

Loading