Open all vlog files in RDWR mode

[jarifibrahim]

With this commit, all log files are opened in read-write mode. Earlier,
we would open files in read-write mode for writing and then reopen them
again in read only mode after writing was completed. This approach lead
to unnecessary complexity and issues (see issue #769).

---

This change is 

[martinmr]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

---

value.go, line 135 at r1 (raw file):

	// Acquire lock because fd will become invalid for a bit.
	// Acquiring the lock is bad because, while we don't hold the lock for a long time, it forces
	// one batch of readers wait for the preceding batch of readers to finish.

Can you rephrase this comment a bit? I find it confusing. First, it says the lock will be acquired but then it says that's bad. Is the reason for it that the fd will become invalid? If so, the comment should say first that acquiring the lock is bad, then explain the reason why we are doing it anyways.

---

value.go, line 800 at r1 (raw file):

	return nil
}
func (vlog *valueLog) openLogFile(lf *logFile) error {

minor: add a blank line above this line

[jarifibrahim]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @manishrjain, and @martinmr)

---

value.go, line 135 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	// Acquire lock because fd will become invalid for a bit.
	// Acquiring the lock is bad because, while we don't hold the lock for a long time, it forces
	// one batch of readers wait for the preceding batch of readers to finish.

Can you rephrase this comment a bit? I find it confusing. First, it says the lock will be acquired but then it says that's bad. Is the reason for it that the fd will become invalid? If so, the comment should say first that acquiring the lock is bad, then explain the reason why we are doing it anyways.

I think the comment was added a while ago and since we're no longer opening and closing the file we don't need the lock. I've removed the lock and the comment.

[manishrjain]

Got a comment.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jarifibrahim and @martinmr)

---

value.go, line 138 at r2 (raw file):

	if err := lf.fd.Truncate(int64(offset)); err != nil {
		return errors.Wrapf(err, "Unable to truncate file: %q", lf.path)
	}

Add a comment here saying that we previously used to reopen files in read only mode, but no longer do so. We keep the log files in RW mode.

---

value.go, line 740 at r2 (raw file):

		// Open log file "lf" in read-write mode
		if err := vlog.openLogFile(lf); err != nil {

Could be lf.open(vlog.opt)?

---

value.go, line 796 at r2 (raw file):

}

func (vlog *valueLog) openLogFile(lf *logFile) error {

This method can be on the logfile struct, like the openReadOnly was.

---

value.go, line 803 at r2 (raw file):

		flags |= y.ReadOnly
		// Set sync flag only for the last vlog file.
	case vlog.opt.SyncWrites && lf.fid == vlog.maxFid:

Now that you're not re-opening the files in read only mode, then all files (even the penultimate log files) can be opened with y.Sync flag, if SyncWrites is true, right?

[jarifibrahim]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @martinmr)

---

value.go, line 138 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a comment here saying that we previously used to reopen files in read only mode, but no longer do so. We keep the log files in RW mode.

Added the comment.

---

value.go, line 740 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Could be lf.open(vlog.opt)?

I'll need to file path also. Changed the function signature to lf.Open(path, flags)

---

value.go, line 796 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This method can be on the logfile struct, like the openReadOnly was.

Done.

---

value.go, line 803 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Now that you're not re-opening the files in read only mode, then all files (even the penultimate log files) can be opened with y.Sync flag, if SyncWrites is true, right?

We won't be writing to any other file except the last one so I thought it we shouldn't unnecessarily open files with sync flag.

Do you think we should open all files in sync mode? Even when we won't be writing to them?

[jarifibrahim]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @martinmr)

---

value.go, line 803 at r2 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We won't be writing to any other file except the last one so I thought it we shouldn't unnecessarily open files with sync flag.

Do you think we should open all files in sync mode? Even when we won't be writing to them?

Changed the code. Now we open all files with sync mode.

Addressed all the comments.