implement /state on alpha; login required if ACL enabled on alpha; al… #4435

parasssh · 2019-12-17T22:55:04Z

…low otherwise

This change is

…low otherwise

golangcibot · 2019-12-17T22:57:52Z

dgraph/cmd/alpha/run.go

+		return
+	}
+
+	w.Write(aResp.Json)


Error return value of w.Write is not checked (from errcheck)

golangcibot · 2019-12-17T22:57:52Z

dgraph/cmd/alpha/run.go

+	}
+
+	w.Write(aResp.Json)
+	return


S1023: redundant return statement (from gosimple)

… state interface

golangcibot · 2019-12-18T21:05:24Z

dgraph/cmd/zero/zero.go

@@ -781,3 +781,8 @@ func (s *Server) applyLicense(ctx context.Context, signedData io.Reader) error {
 	glog.Infof("Enterprise license state proposed to the cluster")
 	return nil
 }
+
+// CurrentState return the current membership state.
+func (s *Server) CurrentState(ctx context.Context, _ *api.Payload) (resp *pb.MembershipState, err error) {


line is 106 characters (from lll)

parasssh

Dismissed @golangcibot from 3 discussions.
Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

danielmai

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 1 of 3 files at r4.
Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/alpha/run.go, line 413 at r4 (raw file):

	http.HandleFunc("/alter", alterHandler)
	http.HandleFunc("/health", healthCheck)
	http.HandleFunc("/state", state)

Call this stateHandler to be consistent with the other names.

edgraph/access_ee.go, line 747 at r4 (raw file):

			}
		}
		// Dont allow non groot to do the State API.

"Don't"

edgraph/server.go, line 591 at r4 (raw file):

authorize int

is this argument used somewhere?

protos/pb.proto, line 456 at r4 (raw file):

	rpc UpdateMembership (Group)                 returns (api.Payload) {}
	rpc StreamMembership (api.Payload)       returns (stream MembershipState) {}
	rpc CurrentState (api.Payload)       returns (MembershipState) {}

Can you simplify the name to "State"?

parasssh

Reviewable status: 1 of 7 files reviewed, 4 unresolved discussions (waiting on @danielmai and @manishrjain)

dgraph/cmd/alpha/run.go, line 413 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Call this stateHandler to be consistent with the other names.

Done.

edgraph/access_ee.go, line 747 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…

"Don't"

Done.

edgraph/server.go, line 591 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…

authorize int
is this argument used somewhere?

Good catch. Using this as flag to check for Authorization.

protos/pb.proto, line 456 at r4 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Can you simplify the name to "State"?

Done.

parasssh

Dismissed @danielmai from 4 discussions.
Reviewable status: 1 of 7 files reviewed, all discussions resolved (waiting on @danielmai and @manishrjain)

manishrjain

Reviewable status: 1 of 7 files reviewed, 6 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/alpha/run.go, line 310 at r5 (raw file):

	ctx := context.Background()
	ctx = attachAccessJwt(ctx, r)

(context.Background(), r)

dgraph/cmd/alpha/run.go, line 313 at r5 (raw file):

	var aResp *api.Response
	if aResp, err = (&edgraph.Server{}).State(ctx); err != nil {

Alpha already has state: gr.GetMembershipState.

dgraph/cmd/zero/zero.go, line 787 at r5 (raw file):

// CurrentState return the current membership state.
func (s *Server) State(ctx context.Context, _ *api.Payload) (*pb.MembershipState, error) {
	return s.latestMembershipState(ctx)

No need. Alpha already has membership state.

edgraph/server.go, line 605 at r5 (raw file):

	// Get the state info from Zero's CurrentState GRPC interface.
	pl := conn.GetPools().Connect(x.WorkerConfig.ZeroAddr)

There is another method available to connect to Zero. Also, I wonder if this is the right place to call Zero directly, or should this be in worker.

edgraph/server.go, line 607 at r5 (raw file):

	pl := conn.GetPools().Connect(x.WorkerConfig.ZeroAddr)
	c := pb.NewZeroClient(pl.Get())
	ms, err := c.State(ctx, &api.Payload{})

No need to get this state. Alpha already has it.

protos/pb.proto, line 456 at r5 (raw file):

	rpc UpdateMembership (Group)                 returns (api.Payload) {}
	rpc StreamMembership (api.Payload)       returns (stream MembershipState) {}
	rpc State (api.Payload)       returns (MembershipState) {}

No need. Connext gives you this already. Also, actually, Alpha already has the latest membership state. It gets it from Zero every second.

This reverts commit 863f6ad.

This reverts commit 5c4763d.

… Alpha's state interface" This reverts commit e6c2c6b.

parasssh

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/alpha/run.go, line 310 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

(context.Background(), r)

Removed this as alpha has state info already.

dgraph/cmd/alpha/run.go, line 313 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Alpha already has state: gr.GetMembershipState.

Done.

dgraph/cmd/zero/zero.go, line 787 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need. Alpha already has membership state.

Done.

edgraph/server.go, line 605 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

There is another method available to connect to Zero. Also, I wonder if this is the right place to call Zero directly, or should this be in worker.

Removed this as alpha has state info already.

edgraph/server.go, line 607 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need to get this state. Alpha already has it.

Done.

parasssh

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @danielmai and @manishrjain)

protos/pb.proto, line 456 at r5 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need. Connext gives you this already. Also, actually, Alpha already has the latest membership state. It gets it from Zero every second.

Done.

parasssh

Dismissed @manishrjain from 6 discussions.
Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @danielmai)

mangalaman93

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)

dgraph/cmd/alpha/run.go, line 312 at r6 (raw file):

	ctx = attachAccessJwt(ctx, r)

	var aResp *api.Response

is resp already defined somewhere?

edgraph/access_ee.go, line 742 at r6 (raw file):

			return status.Error(codes.Unauthenticated, err.Error())
		default:
			userID = userData[0]

what if userData has 0 length?

edgraph/access_ee.go, line 749 at r6 (raw file):

		// Deny non groot users.
		return status.Error(codes.PermissionDenied, fmt.Sprintf("User is '%v'. "+

This could be moved inside the switch, would be more readable. As of now, there are two default cases.

edgraph/server.go, line 587 at r6 (raw file):

// State handles state requests
func (s *Server) State(ctx context.Context) (*api.Response, error) {
	return s.doState(ctx, NeedAuthorize)

There could just be one function

edgraph/server.go, line 605 at r6 (raw file):

	ms := worker.GetMembershipState()
	if ms == nil {
		return nil, errors.New("No membership state found")

error could be defined at the top

edgraph/server.go, line 611 at r6 (raw file):

	var jsonState bytes.Buffer
	if err := m.Marshal(&jsonState, ms); err != nil {
		return nil, errors.New("Error marshalling state information to JSON")

Isn't there an already defined error that we could use here?

parasssh

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @danielmai, @mangalaman93, and @manishrjain)

dgraph/cmd/alpha/run.go, line 312 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

is resp already defined somewhere?

It is in dgo/api/api.pb.go

edgraph/access_ee.go, line 742 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

what if userData has 0 length?

I dont think that scenario can happen if err == nil after the function extractUserAndGroups(ctx) is called.

edgraph/access_ee.go, line 749 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This could be moved inside the switch, would be more readable. As of now, there are two default cases.

Done.

edgraph/server.go, line 587 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

There could just be one function

It could. However, I stayed consistent with Query/doQuery pattern in this file.

edgraph/server.go, line 605 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

error could be defined at the top

Please elaborate.

edgraph/server.go, line 611 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Isn't there an already defined error that we could use here?

I didnt find any, rest of the file uses errors.Errorf() for inline errors. I'll change to that.

parasssh

Dismissed @mangalaman93 from 6 discussions.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)

dgraph/cmd/alpha/run.go, line 312 at r6 (raw file):

Previously, parasssh wrote…

It is in dgo/api/api.pb.go

Done.

parasssh

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)

edgraph/server.go, line 605 at r6 (raw file):

Previously, parasssh wrote…

Please elaborate.

Defining error at the top of the function is inconsistent and a bit unwieldy. I prefer it inline.

golangcibot · 2019-12-20T18:10:51Z

edgraph/access_ee.go

+			userID = userData[0]
+			if userID == x.GrootId {
+				return nil
+			} else {


if block ends with a return statement, so drop this else and outdent its block (from golint)

danielmai

Reviewed 2 of 6 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @parasssh)

parasssh

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai and @golangcibot)

edgraph/access_ee.go, line 745 at r7 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

if block ends with a return statement, so drop this else and outdent its block (from golint)

Done.

parasssh

Dismissed @golangcibot from a discussion.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @danielmai)

mangalaman93

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai and @parasssh)

dgraph/cmd/alpha/run.go, line 312 at r6 (raw file):

Previously, parasssh wrote…

Done.

Still called aResp. We could just call it resp.

edgraph/access_ee.go, line 743 at r8 (raw file):

		default:
			userID = userData[0]
			if userID == x.GrootId {

This could also be case statement.

edgraph/server.go, line 605 at r6 (raw file):

Previously, parasssh wrote…

Defining error at the top of the function is inconsistent and a bit unwieldy. I prefer it inline.

I think, defining the errors at the top creates the errors only once. Otherwise, the object is created every time increasing GC pressure.

parasssh

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai and @mangalaman93)

edgraph/access_ee.go, line 743 at r8 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

This could also be case statement.

The if is more readable here IMO.

edgraph/server.go, line 605 at r6 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I think, defining the errors at the top creates the errors only once. Otherwise, the object is created every time increasing GC pressure.

I still dont get it. This error is created when the condition is met. Not otherwise. I will sync offline.

implement /state on alpha; login required if ACL enabled on alpha; al…

675894d

…low otherwise

parasssh requested review from manishrjain and danielmai December 17, 2019 22:55

golangcibot reviewed Dec 17, 2019

View reviewed changes

Paras Shah added 2 commits December 17, 2019 15:21

bot fixes

f34d0b4

add CurrentState Zero GRPC interface which in turn is used by Alpha's…

e6c2c6b

… state interface

golangcibot reviewed Dec 18, 2019

View reviewed changes

address GoLangCI and DeepSource comments

5c4763d

parasssh commented Dec 18, 2019

View reviewed changes

parasssh marked this pull request as ready for review December 18, 2019 21:50

parasssh requested a review from a team as a code owner December 18, 2019 21:50

danielmai suggested changes Dec 18, 2019

View reviewed changes

address Daniel's comments

863f6ad

parasssh commented Dec 18, 2019

View reviewed changes

manishrjain suggested changes Dec 19, 2019

View reviewed changes

Paras Shah added 4 commits December 19, 2019 09:58

Revert "address Daniel's comments"

9f4f864

This reverts commit 863f6ad.

Revert "address GoLangCI and DeepSource comments"

f20370c

This reverts commit 5c4763d.

Revert "add CurrentState Zero GRPC interface which in turn is used by…

adc0973

… Alpha's state interface" This reverts commit e6c2c6b.

use state info that is already on alpha instead of getting it from zero

969ecdf

parasssh commented Dec 19, 2019

View reviewed changes

parasssh requested review from manishrjain and danielmai December 19, 2019 18:44

mangalaman93 requested changes Dec 20, 2019

View reviewed changes

parasssh commented Dec 20, 2019

View reviewed changes

address Aman's comments

5614b7d

parasssh commented Dec 20, 2019

View reviewed changes

golangcibot reviewed Dec 20, 2019

View reviewed changes

danielmai approved these changes Dec 20, 2019

View reviewed changes

go lint

2f82c2f

parasssh commented Dec 20, 2019

View reviewed changes

mangalaman93 requested a review from danielmai December 21, 2019 09:18

mangalaman93 approved these changes Dec 21, 2019

View reviewed changes

parasssh commented Dec 23, 2019

View reviewed changes

parasssh merged commit b36859e into master Dec 23, 2019

parasssh deleted the ps_alpha_state_endpoint branch January 30, 2020 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement /state on alpha; login required if ACL enabled on alpha; al… #4435

implement /state on alpha; login required if ACL enabled on alpha; al… #4435

parasssh commented Dec 17, 2019 •

edited by manishrjain

Loading

golangcibot Dec 17, 2019

golangcibot Dec 17, 2019

golangcibot Dec 18, 2019

parasssh left a comment

danielmai left a comment

parasssh left a comment

parasssh left a comment

manishrjain left a comment

parasssh left a comment

parasssh left a comment

parasssh left a comment

mangalaman93 left a comment

parasssh left a comment

parasssh left a comment

parasssh left a comment

golangcibot Dec 20, 2019

danielmai left a comment

parasssh left a comment

parasssh left a comment

mangalaman93 left a comment

parasssh left a comment

implement /state on alpha; login required if ACL enabled on alpha; al… #4435

implement /state on alpha; login required if ACL enabled on alpha; al… #4435

Conversation

parasssh commented Dec 17, 2019 • edited by manishrjain Loading

golangcibot Dec 17, 2019

Choose a reason for hiding this comment

golangcibot Dec 17, 2019

Choose a reason for hiding this comment

golangcibot Dec 18, 2019

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

danielmai left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

golangcibot Dec 20, 2019

Choose a reason for hiding this comment

danielmai left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

mangalaman93 left a comment

Choose a reason for hiding this comment

parasssh left a comment

Choose a reason for hiding this comment

parasssh commented Dec 17, 2019 •

edited by manishrjain

Loading