Add pid.lock file locking in the opt.Dir directory

[srh]

Adds a pid.lock file.

Connects to hypermodeinc/dgraph#1185

---

This change is 

[manishrjain]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

kv.go, line 150 at r1 (raw file):

		}
	}
	if err := createPidFile(opt.Dir); err != nil {

call it acquireLockFile or createLockFile

---

kv.go, line 329 at r1 (raw file):

const (
	pidFileName        = "pid.lock"

call it LOCK (which is what RocksDB calls it). Also, lockFile (Name suffix isn't required because we don't have a lockFile as a file pointer).

---

kv.go, line 330 at r1 (raw file):

const (
	pidFileName        = "pid.lock"
	cannotCreateErrMsg = "cannot create pid lock file"

No need for this variable. Just put the contents directly in errors.Wrap(..)

---

kv.go, line 339 at r1 (raw file):

		return errors.Wrap(err, cannotCreateErrMsg)
	}
	_, err = fmt.Fprintf(f, "%d\n", os.Getpid())

ioutil.Write

---

kv.go, line 345 at r1 (raw file):

	}
	if closeErr != nil {
		return errors.Wrap(err, "cannot close pid lock file")

closeErr

---

kv.go, line 352 at r1 (raw file):

// destroyPidFile removes the pid file.  Don't forget to syncDir after this, to ensure the changes
// are permanently written to the file system.
func destroyPidFile(dir string) error {

This func gets called exactly once. Might as well invoke the logic directly from the caller.

os.Remove(filepath.Join(dir, pidFileName))

---

kv.go, line 358 at r1 (raw file):

// When you create or delete a file, you have to ensure the directory entry for the file is synced
// in order to guarantee the file is visible (if the system crashes).

Is this the case? Any relevant documentation links would be useful.

---

Comments from Reviewable

[srh]

Review status: 1 of 3 files reviewed at latest revision, 7 unresolved discussions.

---

kv.go, line 150 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

call it acquireLockFile or createLockFile

Done.

---

kv.go, line 329 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

call it LOCK (which is what RocksDB calls it). Also, lockFile (Name suffix isn't required because we don't have a lockFile as a file pointer).

Done.

---

kv.go, line 330 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need for this variable. Just put the contents directly in errors.Wrap(..)

Okay, then kv_test will have a copy.

---

kv.go, line 339 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

ioutil.Write

You mean ioutil.WriteFile? It's no good because we need to O_EXCL.

---

kv.go, line 345 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

closeErr

Done.

---

kv.go, line 352 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This func gets called exactly once. Might as well invoke the logic directly from the caller.

os.Remove(filepath.Join(dir, pidFileName))

Reasonable if createLockFile takes the full path directly too. Done.

---

kv.go, line 358 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Is this the case? Any relevant documentation links would be useful.

Yes, definitely. See the man page for fsync. Also see for example etcd-io/etcd#6368 .

---

Comments from Reviewable

[manishrjain]

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

kv.go, line 339 at r1 (raw file):

Previously, srh (Sam Hughes) wrote…

You mean ioutil.WriteFile? It's no good because we need to O_EXCL.

OK. Actually meant to remove that comment, somehow slipped through.

---

kv.go, line 358 at r1 (raw file):

Previously, srh (Sam Hughes) wrote…

Yes, definitely. See the man page for fsync. Also see for example etcd-io/etcd#6368 .

Can you add this link as a comment here. Because this means, every time we create a new file, we need to do this dir sync? That'd require more work.

---

Comments from Reviewable

[manishrjain]

Address the comment before submitting.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[srh]

In master as of e13ddb2.