Remove DebugMode option.

[martinmr]

This alpha option is not doing too much so it's being removed and
replaced with calls to glog.V(2) where appropriate. Since the change
made x.Errorf and x.Wrapf just wrappers of the existing methods in the
errors package, this PR also removes these methods and replaces them
with the methods in the errors package.

---

This change is 

[danielmai]

Reviewed 1 of 69 files at r1.
Reviewable status: 1 of 68 files reviewed, 17 unresolved discussions (waiting on @campoy, @danielmai, @golangcibot, and @martinmr)

---

x/keys.go, line 435 at r3 (raw file):

	case ByteData, ByteReverse:
		if len(k) < 8 {
			glog.V(2).Infof("Error: Uid length < 8 for key: %q, parsed key: %+v\n", key, p)

These should use glog.Errorf since it's an error message that should not happen.

---

x/keys.go, line 445 at r3 (raw file):

		if len(k) < 16 {
			glog.V(2).Infof("Error: StartUid length < 8 for key: %q, parsed key: %+v\n", key, p)

These should use glog.Errorf since it's an error message that should not happen.

---

x/keys.go, line 458 at r3 (raw file):

		if len(k) < 8 {
			glog.V(2).Infof("Error: StartUid length < 8 for key: %q, parsed key: %+v\n", key, p)

These should use glog.Errorf since it's an error message that should not happen.

---

x/keys.go, line 468 at r3 (raw file):

	case ByteCount, ByteCountRev:
		if len(k) < 4 {
			glog.V(2).Infof("Error: Count length < 4 for key: %q, parsed key: %+v\n", key, p)

These should use glog.Errorf since it's an error message that should not happen.

---

x/keys.go, line 478 at r3 (raw file):

		if len(k) < 12 {
			glog.V(2).Infof("Error: StartUid length < 8 for key: %q, parsed key: %+v\n", key, p)

These should use glog.Errorf since it's an error message that should not happen.

[martinmr]

Reviewable status: 0 of 68 files reviewed, 17 unresolved discussions (waiting on @campoy, @danielmai, and @golangcibot)

---

conn/node.go, line 631 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 101 characters (from lll)

Done.

---

dgraph/cmd/zero/zero.go, line 404 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of errors.Errorf is not checked (from errcheck)

Done.

---

worker/draft.go, line 760 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of errors.Errorf is not checked (from errcheck)

Done.

---

worker/mutation.go, line 38 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

---

worker/predicate_move.go, line 27 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

---

worker/proposal.go, line 34 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

---

worker/sort.go, line 89 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 103 characters (from lll)

Done.

---

worker/sort.go, line 176 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 102 characters (from lll)

Done.

---

worker/sort.go, line 198 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 103 characters (from lll)

Done.

---

worker/sort.go, line 401 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

line is 101 characters (from lll)

Done.

---

x/keys.go, line 435 at r3 (raw file):

Previously, danielmai (Daniel Mai) wrote…

These should use glog.Errorf since it's an error message that should not happen.

Done.

---

x/keys.go, line 445 at r3 (raw file):

Previously, danielmai (Daniel Mai) wrote…

These should use glog.Errorf since it's an error message that should not happen.

Done.

---

x/keys.go, line 458 at r3 (raw file):

Previously, danielmai (Daniel Mai) wrote…

These should use glog.Errorf since it's an error message that should not happen.

Done.

---

x/keys.go, line 468 at r3 (raw file):

Previously, danielmai (Daniel Mai) wrote…

These should use glog.Errorf since it's an error message that should not happen.

Done.

---

x/keys.go, line 478 at r3 (raw file):

Previously, danielmai (Daniel Mai) wrote…

These should use glog.Errorf since it's an error message that should not happen.

Done.

addressed review

[campoy]

Reviewed 18 of 69 files at r1, 2 of 5 files at r3.
Reviewable status: 20 of 68 files reviewed, 27 unresolved discussions (waiting on @campoy, @danielmai, @golangcibot, and @martinmr)

---

chunker/rdf/parse.go, line 175 at r3 (raw file):

			it.Prev() // backup '('
			if err := parseFacets(it, &rnq); err != nil {
				return rnq, errors.Errorf(err.Error())

Every time you write code like this think about whether it'd make more sense to use errors.Wrap or errors.Wrapf.
For instance, I'd consider writing errors.Wrapf(err, "could not parse facets")

Find all places where you were calling Errorf and passing an error as parameter and use Wrapf instead.

---

chunker/rdf/parse.go, line 182 at r3 (raw file):

---

conn/raft_server.go, line 252 at r3 (raw file):

				glog.Warningf("Error while raft.Step from %#x: %v. Closing RaftMessage stream.",
					rc.GetId(), err)
				return errors.Errorf("Error while raft.Step from %#x: %v", rc.GetId(), err)

Use Wrapf

---

dgraph/cmd/cert/create.go, line 28 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

goimports has been disabled.

why?

---

dgraph/cmd/zero/oracle.go, line 351 at r3 (raw file):

			gid, err := strconv.Atoi(splits[0])
			if err != nil {
				return errors.Errorf("Unable to parse group id from %s. Error: %v", pkey, err)

Wrapf

---

dgraph/cmd/zero/tablet.go, line 100 at r3 (raw file):

	// Ensure that I'm connected to the rest of the Zero group, and am the leader.
	if _, err := s.latestMembershipState(ctx); err != nil {
		return errors.Errorf("Unable to reach quorum: %v", err)

Wrapf

---

dgraph/cmd/zero/tablet.go, line 122 at r3 (raw file):

	ids, err := s.Timestamps(ctx, &pb.Num{Val: 1})
	if err != nil || ids.StartId == 0 {
		return errors.Errorf("While leasing txn timestamp. Id: %+v Error: %v", ids, err)

Wrapf

---

dgraph/cmd/zero/tablet.go, line 154 at r3 (raw file):

	glog.Info(msg)
	if err := s.Node.proposeAndWait(ctx, p); err != nil {
		return errors.Errorf("While proposing tablet reassignment. Proposal: %+v Error: %v", p, err)

Wrapf

---

edgraph/server.go, line 867 at r3 (raw file):

		}
		if err := validateKeys(nq); err != nil {
			return errors.Errorf("Key error: %s: %+v", err, nq)

Wrapf

---

edgraph/server.go, line 902 at r3 (raw file):

func validateKeys(nq *api.NQuad) error {
	if err := validateKey(nq.Predicate); err != nil {
		return errors.Errorf("predicate %q %s", nq.Predicate, err)

Wrapf

---

ee/acl/utils.go, line 86 at r3 (raw file):

	err = json.Unmarshal(resp.GetJson(), &m)
	if err != nil {
		return nil, fmt.Errorf("unable to unmarshal the query user response:%v", err)

errors.Wrapf

You should replace all fmt.Errorf calls with errors.Errorf or errors.Wrapf.

[martinmr]

Reviewable status: 13 of 68 files reviewed, 26 unresolved discussions (waiting on @campoy, @danielmai, and @golangcibot)

---

chunker/rdf/parse.go, line 175 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Every time you write code like this think about whether it'd make more sense to use errors.Wrap or errors.Wrapf.
For instance, I'd consider writing errors.Wrapf(err, "could not parse facets")

Find all places where you were calling Errorf and passing an error as parameter and use Wrapf instead.

Done. I'll replace the lines I modified in this PR to use wrapf where appropriate. I'll do the rest later to keep this PR from getting too big.

---

conn/raft_server.go, line 252 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Use Wrapf

Done.

---

dgraph/cmd/cert/create.go, line 28 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

why?

There were conflicting warnings between gofmt and goimports. Since gofmt is already fixing the formatting of the imports I kept it.

---

dgraph/cmd/zero/oracle.go, line 351 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

dgraph/cmd/zero/tablet.go, line 100 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

dgraph/cmd/zero/tablet.go, line 122 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

dgraph/cmd/zero/tablet.go, line 154 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

edgraph/server.go, line 867 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

edgraph/server.go, line 902 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

ee/acl/utils.go, line 86 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

errors.Wrapf

You should replace all fmt.Errorf calls with errors.Errorf or errors.Wrapf.

Done. I'll change the rest in a different PR to prevent this PR from getting too big. I have opened an issue in github.

Addressed changes

[campoy]

Reviewed 37 of 69 files at r1, 8 of 9 files at r2, 2 of 5 files at r3, 1 of 1 files at r4, 8 of 8 files at r5.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @campoy, @danielmai, @golangcibot, and @martinmr)

---

edgraph/server.go, line 867 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Done.

error text should be lower case, and you dropped one space after the colon:

"key error: %+v"

---

ee/backup/run.go, line 253 at r5 (raw file):

	manifests, err := ListManifests(opt.location)
	if err != nil {
		return errors.Errorf("Error while listing manifests: %v", err.Error())

Wrapf

---

ee/backup/s3_handler.go, line 135 at r5 (raw file):

	found, err := mc.BucketExists(h.bucketName)
	if err != nil {
		return nil, errors.Errorf("Error while looking for bucket: %s at host: %s. Error: %v",

Wrapf

---

gql/parser.go, line 324 at r5 (raw file):

			case "string": // Value is a valid string. No checks required.
			default:
				return errors.Errorf("Type %v not supported", typ)

I'd use %q instead of %v just in case typ is an empty string or worse

---

posting/mvcc.go, line 35 at r5 (raw file):

var (
	ErrTsTooOld = errors.Errorf("Transaction is too old")

This doesn't seem to be used anywhere. Maybe we can simply remove it?

---

query/aggregator.go, line 111 at r5 (raw file):

		return !isEqual, nil
	}
	return false, errors.Errorf("Invalid compare function %v", ag)

use %q just in case the string is empty or not printable

---

query/aggregator.go, line 262 at r5 (raw file):

		}
	default:
		return errors.Errorf("Unhandled aggregator function %v", ag.name)

same, %q

---

systest/cluster_test.go, line 150 at r5 (raw file):

	cmd.Process.Signal(syscall.SIGINT)
	if _, err := cmd.Process.Wait(); err != nil {
		return errors.Errorf("Error while waiting for Dgraph process to be killed: %v", err)

Wrapf

---

systest/cluster_test.go, line 156 at r5 (raw file):

	glog.Infoln("Trying to restart Dgraph Alpha")
	if err := cmd.Start(); err != nil {
		return errors.Errorf("Couldn't start Dgraph alpha again: %v\n", err)

Wrapf

---

worker/draft.go, line 760 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Done.

This is a bit weird, we're not returning the error at all.

---

worker/draft.go, line 545 at r5 (raw file):

	}
	if err := writer.Flush(); err != nil {
		return errors.Errorf("Error while flushing to disk: %v", err)

Wrapf

---

worker/mutation.go, line 272 at r5 (raw file):

		if t.Enum() != s.ValueType {
			return errors.Errorf("Schema change not allowed from %s to %s",
				t.Enum().String(), typ.Enum().String())

the String() is unnecessary, as the errors package will use fmt which knows about the Stringer interface

---

worker/sort.go, line 174 at r5 (raw file):

	if err != nil {
		return &sortresult{&emptySortResult, nil,
			fmt.Errorf("Attribute %s not defined in schema", order.Attr)}

errors

---

x/error.go, line 99 at r5 (raw file):

// Wrap wraps errors from external lib.
func Wrap(err error) error {

Shouldn't we be able to also remove this one?

[campoy]

Reviewable status: all files reviewed, 30 unresolved discussions (waiting on @campoy, @danielmai, @golangcibot, and @martinmr)

---

ee/backup/run.go, line 253 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

not fixed

---

ee/backup/s3_handler.go, line 135 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

And change error message so it doesn't include the word error.

[martinmr]

Reviewable status: 43 of 66 files reviewed, 29 unresolved discussions (waiting on @campoy, @danielmai, and @golangcibot)

---

edgraph/server.go, line 867 at r3 (raw file):

Previously, campoy (Francesc Campoy) wrote…

error text should be lower case, and you dropped one space after the colon:

"key error: %+v"

Done.

---

ee/backup/run.go, line 253 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

not fixed

Done.

---

ee/backup/s3_handler.go, line 135 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

And change error message so it doesn't include the word error.

Done.

---

gql/parser.go, line 324 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

I'd use %q instead of %v just in case typ is an empty string or worse

Done.

---

posting/mvcc.go, line 35 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

This doesn't seem to be used anywhere. Maybe we can simply remove it?

There's another PR to remove unused vars. It should be removed in that one but I'll check.

---

query/aggregator.go, line 111 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

use %q just in case the string is empty or not printable

Done.

---

query/aggregator.go, line 262 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

same, %q

Done.

---

systest/cluster_test.go, line 150 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

systest/cluster_test.go, line 156 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

worker/draft.go, line 760 at r1 (raw file):

Previously, campoy (Francesc Campoy) wrote…

This is a bit weird, we're not returning the error at all.

This is inside of a for loop meant to run constantly so an error cannot be returned here.

---

worker/draft.go, line 545 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Wrapf

Done.

---

worker/mutation.go, line 272 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

the String() is unnecessary, as the errors package will use fmt which knows about the Stringer interface

Done.

---

worker/sort.go, line 174 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

errors

Done.

---

x/error.go, line 99 at r5 (raw file):

Previously, campoy (Francesc Campoy) wrote…

Shouldn't we be able to also remove this one?

Done.

addressed comments.

[campoy]

Reviewed 23 of 25 files at r6.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @danielmai, @golangcibot, and @martinmr)

---

conn/node.go, line 737 at r6 (raw file):

		// There exists a healthy connection to server with same id.
		if _, err := GetPools().Get(addr); err == nil {
			return &api.Payload{}, errors.Errorf(

Why is this not returning nil instead of &api.Payload{}?

---

query/mutation.go, line 182 at r6 (raw file):

		edge, err = wnq.ToEdgeUsing(newUids)
		if err != nil {
			return errors.Wrap(err, "")

Just return err

[campoy]

LGTM as long as you verify that the changes unrelated to this PR are indeed result of the previous merges.

Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @danielmai, @golangcibot, and @martinmr)

[campoy]

Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @danielmai, @golangcibot, and @martinmr)

[campoy]

Reviewed 15 of 16 files at r7.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @danielmai, @golangcibot, and @martinmr)

[martinmr]

I checked and the only unrelated change was a typechange (from link to regular file) that I have reverted. Merging now.