Rebalance missing validation and code refactor #986

rishubhjain · 2018-07-10T16:44:38Z

Issue: #428

rishubhjain · 2018-07-10T16:52:24Z

Remaining :

CLI
E2E
TODOs.

Will update the PR with the remaining sections.

Madhu-1

please update endpoints.md file

Madhu-1 · 2018-07-11T04:57:57Z

plugins/rebalance/init.go

+ Pattern: "/volumes/{volname}/rebalance/start",
+ Version: 1,
+ RequestType: utils.GetTypeString((*rebalanceapi.StartReq)(nil)),
+ ResponseType: utils.GetTypeString((*uuid.UUID)(nil)),


instead of giving an ID to the user, send it as a JSON struct or send RebalStatus or RebalInfo to user,

type RebalanceStartResp struct{ ID string }

Madhu-1 · 2018-07-11T06:04:47Z

plugins/rebalance/rest.go

- rebalinfo := createRebalanceInfo(volname, &req)
- if rebalinfo == nil {
- logger.WithError(err).Error("failed to create Rebalance info")
+ rebalInfo, err := GetRebalanceInfo(volname)


are you missing error handling here?

No I want the error to be not nil

what happens when we get err !=nil don't we need to stop proceeding further
and return the error to the user

Madhu-1 · 2018-07-11T06:06:24Z

plugins/rebalance/rest.go

+ }
+
+ rebalInfo = createRebalanceInfo(volname, &req)
+ if rebalInfo == nil {


question: in what scenario createRebalanceInfo will return nil?

I think this scenario won't happen, but I have kept the error handling in case of future use.

if this is for future use, this check will become redundant now. then we need to comment the code or remove it

Madhu-1 · 2018-07-11T06:10:41Z

plugins/rebalance/rest.go

 if err != nil {
- logger.WithError(err).Error("failed to set rebalance info in transaction context")
+ logger.WithError(err).WithField("key", "rinfo").Error("failed to set rebalance info in transaction context")


same question about logging as above.

Already answered in your PR

Madhu-1 · 2018-07-11T06:20:54Z

plugins/rebalance/rest.go

@@ -162,14 +171,14 @@ func rebalanceStopHandler(w http.ResponseWriter, r *http.Request) {
 return
 }

- rebalinfo, err := GetRebalanceInfo(volname)
+ rebalInfo, err := GetRebalanceInfo(volname)


question:
do we need to return ErrRebalanceNotStarted error if get from store fails?

Madhu-1 · 2018-07-11T06:22:21Z

plugins/rebalance/rest.go

@@ -162,14 +171,14 @@ func rebalanceStopHandler(w http.ResponseWriter, r *http.Request) {
 return
 }

- rebalinfo, err := GetRebalanceInfo(volname)
+ rebalInfo, err := GetRebalanceInfo(volname)
 if err != nil {
 restutils.SendHTTPError(r.Context(), w, http.StatusBadRequest, ErrRebalanceNotStarted)


as ctx is already defined as ctx := r.Context() ,send ctx instead of r.Context() to restutils.SendHTTPError

Madhu-1 · 2018-07-11T06:23:07Z

plugins/rebalance/rest.go

@@ -188,34 +197,33 @@ func rebalanceStopHandler(w http.ResponseWriter, r *http.Request) {

 err = txn.Ctx.Set("volname", volname)
 if err != nil {
- logger.WithError(err).Error("failed to set volname in transaction context")
+ logger.WithError(err).WithField("key", "volname").Error("failed to set volname in transaction context")


same WithField question for logging in this PR?

Madhu-1 · 2018-07-11T06:25:24Z

plugins/rebalance/rest.go

- rebalinfo.Volname = volname
- rebalinfo.State = rebalanceapi.Stopped
- rebalinfo.Cmd = rebalanceapi.CmdStop
+ rebalInfo.Volname = volname


question:

do we need to assign rebalInfo.Volname = volname as Volname already assigned during rebalance start?

Madhu-1 · 2018-07-11T06:48:23Z

@rishubhjain please add a TODO for UNDO functions in txn steps for rebalance start and stop.

…balance

Madhu-1 · 2018-07-12T09:39:10Z

glustercli/cmd/rebalance.go

+)
+
+var rebalanceCmd = &cobra.Command{
+ Use: "volume rebalance",


where ever its required place add Long command to give more information about this CLI command to users

Madhu-1 · 2018-07-12T09:41:09Z

glustercli/cmd/rebalance.go

+ volname := cmd.Flags().Args()[0]
+ var err error
+ if flagRebalanceStartCmdForce && flagRebalanceStartCmdFixLayout {
+ err := errors.New("Conflicting options found")


use above declared err variable here, start error with small case

Madhu-1 · 2018-07-12T09:43:40Z

glustercli/cmd/rebalance.go

+ if err != nil {
+ if verbose {
+ log.WithError(err.Error()).WithFields(log.Fields{
+ "volume": volname,


question:
do we need to convert err.Error() error to string before logging?

err.Error() itself is a string.

log.WithError does not expect input as string, you need to pass error to it

Madhu-1 · 2018-07-12T09:57:52Z

plugins/rebalance/rest.go

- rebalinfo := createRebalanceInfo(volname, &req)
- if rebalinfo == nil {
- logger.WithError(err).Error("failed to create Rebalance info")
+ rebalInfo, err := GetRebalanceInfo(volname)


what happens when we get err !=nil don't we need to stop proceeding further
and return the error to the user

Madhu-1 · 2018-07-12T09:59:22Z

plugins/rebalance/rest.go

+ }
+
+ rebalInfo = createRebalanceInfo(volname, &req)
+ if rebalInfo == nil {


if this is for future use, this check will become redundant now. then we need to comment the code or remove it

Madhu-1 · 2018-07-12T09:59:55Z

plugins/rebalance/rest.go

@@ -118,25 +127,16 @@ func rebalanceStartHandler(w http.ResponseWriter, r *http.Request) {
 * rebalance process is one per node per volume.
 * Need to handle scenarios where process is started in
 * few nodes and failed in few others */
- logger.WithFields(log.Fields{
- "error": err.Error(),
+ logger.WithError(err).WithFields(log.Fields{


use WithField if you have only one field to log

Madhu-1 · 2018-07-12T10:00:44Z

plugins/rebalance/rest.go

 restutils.SendHTTPError(ctx, w, http.StatusInternalServerError, err.Error())
 return
 }

 err = txn.Do()
 if err != nil {
- logger.WithFields(log.Fields{
- "error": err.Error(),
+ logger.WithError(err).WithFields(log.Fields{


use WithField to log if you have only one field to log

Madhu-1 · 2018-07-12T10:01:29Z

plugins/rebalance/rest.go

 return
 }

 err = txn.Do()
 if err != nil {
- logger.WithFields(log.Fields{
- "error": err.Error(),
+ logger.WithError(err).WithFields(log.Fields{


same here log with WithField

Madhu-1 · 2018-07-12T10:11:47Z

pkg/restclient/rebalance.go

+ req := rebalanceapi.StartReq{
+ Option: option,
+ }
+ url := fmt.Sprintf("/v1/volumes/%s/rebalance/start", volname)


missing import fmt package

Madhu-1 · 2018-07-12T10:12:08Z

pkg/restclient/rebalance.go

+ Option: option,
+ }
+ url := fmt.Sprintf("/v1/volumes/%s/rebalance/start", volname)
+ return c.post(url, req, http.StatusOK, nil)


same here, where are you importing HTTP package?

vpandey-RH · 2018-07-12T12:03:23Z

glustercli/cmd/rebalance.go

+func init() {
+
+ // Rebalance Start
+ rebalanceStartCmd.Flags().BoolVar(&flagRebalanceStartCmdForce, "force", false, "Force")


The description of the flags can be a little bit more elaborate.

For now I am following the trend set by volumestart, will update both of them with more description in the next PR

…balance

nbalacha · 2018-07-19T07:57:39Z

glustercli/cmd/rebalance.go

+const (
+ helpRebalanceCmd = "Gluster Rebalance"
+ helpRebalanceStartCmd = "Start rebalance session for gluster volume"
+ helpRebalanceStatusCmd = "Status of rebalance seesion"


Nit: typo (seesion)

Please use rebalance operation instead of rebalance session.

nbalacha · 2018-07-19T08:03:40Z

plugins/rebalance/rest.go

@@ -44,14 +44,18 @@ func rebalanceStartHandler(w http.ResponseWriter, r *http.Request) {
 return
 }

- rebalinfo := createRebalanceInfo(volname, &req)
- if rebalinfo == nil {
- logger.WithError(err).Error("failed to create Rebalance info")


Does this require to log the volume name as well?

IMO : Its easier to debug if the resource name is provided with error/debug message

nbalacha · 2018-07-19T08:07:18Z

plugins/rebalance/rest.go

 )

- err := ctx.Get("rinfo", &rebalinfo)
+ err := ctx.Get("rinfo", &rebalInfo)
 if err != nil {


Rebalance info would be more consistent across err messages.

…balance

atinmu · 2018-07-20T14:15:01Z

On Fri, 20 Jul 2018 at 19:32, Rishubh Jain ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugins/rebalance/rest.go <#986 (comment)>: > @@ -44,14 +44,18 @@ func rebalanceStartHandler(w http.ResponseWriter, r *http.Request) { return } - rebalinfo := createRebalanceInfo(volname, &req) - if rebalinfo == nil { - logger.WithError(err).Error("failed to create Rebalance info") IMO : Its easier to debug if the resource name is provided with error/debug message

What’s the resource name represent here? Isn’t that supposed to be volume name? —

You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#986 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGp7mEcHkbYQhWDT1HB4blpGsPr9hepRks5uIeLvgaJpZM4VJwat> .

-- - Atin (atinm)

…balance

rishubhjain · 2018-07-23T07:00:19Z

retest this please

prashanthpai · 2018-07-27T04:20:10Z

doc/endpoints.md

@@ -81,14 +79,16 @@ WebhookTest | POST | /events/webhook/test | [Webhook](https://godoc.org/github.c
 WebhookDelete | DELETE | /events/webhook | [WebhookDel](https://godoc.org/github.com/gluster/glusterd2/pkg/api#WebhookDel) | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#)
 WebhookList | GET | /events/webhook | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#) | [WebhookList](https://godoc.org/github.com/gluster/glusterd2/pkg/api#WebhookList)
 EventsList | GET | /events | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#) | [Event](https://godoc.org/github.com/gluster/glusterd2/pkg/api#Event)
-SelfHealInfo | GET | /volumes/{name}/{opts}/heal-info | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#) | [BrickHealInfo](https://godoc.org/github.com/gluster/glusterd2/pkg/api#BrickHealInfo)
-SelfHealInfo2 | GET | /volumes/{name}/heal-info | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#) | [BrickHealInfo](https://godoc.org/github.com/gluster/glusterd2/pkg/api#BrickHealInfo)
+SelfHealInfo | GET | /volumes/{volname}/{opts}/heal-info | [](https://godoc.org/github.com/gluster/glusterd2/pkg/api#) | [BrickHealInfo](https://godoc.org/github.com/gluster/glusterd2/pkg/api#BrickHealInfo)


This has to be regenerated

Rebalance code refactor and validation

7a35c21

Madhu-1 suggested changes Jul 11, 2018

View reviewed changes

Merge branch 'master' of https://github.com/gluster/glusterd2 into re…

486e079

…balance

rishubhjain force-pushed the rebalance branch from a7fe8e3 to 486e079 Compare July 11, 2018 12:33

Adding CLI

ef84e98

rishubhjain force-pushed the rebalance branch from 083371d to 201e553 Compare July 12, 2018 09:21

Madhu-1 suggested changes Jul 12, 2018

View reviewed changes

rishubhjain force-pushed the rebalance branch 2 times, most recently from d6308da to 109d636 Compare July 12, 2018 11:36

vpandey-RH reviewed Jul 12, 2018

View reviewed changes

rishubhjain force-pushed the rebalance branch from 109d636 to c17e922 Compare July 12, 2018 13:50

Merge branch 'master' of https://github.com/gluster/glusterd2 into re…

68f8011

…balance

rishubhjain force-pushed the rebalance branch from c17e922 to 599085b Compare July 13, 2018 04:06

Merge branch 'master' of https://github.com/gluster/glusterd2 into re…

6f60a76

…balance

rishubhjain force-pushed the rebalance branch 2 times, most recently from 2e16cd9 to 1f9abf2 Compare July 13, 2018 09:07

aravindavk added the GCS-Sprint2-review-pending label Jul 18, 2018

nbalacha reviewed Jul 19, 2018

View reviewed changes

Merge branch 'master' of https://github.com/gluster/glusterd2 into re…

a6c3f10

…balance

rishubhjain force-pushed the rebalance branch from 1f9abf2 to e8aa38e Compare July 20, 2018 14:09

Merge branch 'master' of https://github.com/gluster/glusterd2 into re…

acbb488

…balance

rishubhjain force-pushed the rebalance branch from e8aa38e to acbb488 Compare July 20, 2018 18:53

prashanthpai reviewed Jul 27, 2018

View reviewed changes

atinmu added priority: low and removed GCS-Sprint2-review-pending labels Sep 14, 2018

Rebalance missing validation and code refactor #986

Are you sure you want to change the base?

Rebalance missing validation and code refactor #986

Conversation

rishubhjain commented Jul 10, 2018

rishubhjain commented Jul 10, 2018 • edited

Madhu-1 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

Madhu-1 commented Jul 11, 2018 • edited

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

Madhu-1 Jul 12, 2018 • edited

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

atinmu commented Jul 20, 2018 via email

rishubhjain commented Jul 23, 2018

Choose a reason for hiding this comment

rishubhjain commented Jul 10, 2018 •

edited

Madhu-1 commented Jul 11, 2018 •

edited

Madhu-1 Jul 12, 2018 •

edited