Fix Txn too big error in bulk loader

[ashish-goswami]

Fixes #3916

I was able to reproduce this issue after running bulk loader with file generate using below code:

package main

import (
	"fmt"
	"os"
)

func main() {
	f, err := os.Create("./many.rdf")
	if err != nil {
		fmt.Println("unable to open file ", err)
		return
	}
	for i := 0; i < 1000000; i++ {
		a := fmt.Sprintf("_:id%d <pred%d> \"val\" .", i, i)
		f.Write([]byte(a))
		f.Write([]byte("\n"))
	}
	f.Sync()
	f.Close()
}

I was thinking of using TxnWriter

dgraph/posting/writer.go

Lines 29 to 33 in d048d5c

	type TxnWriter struct {
	db *badger.DB
	wg sync.WaitGroup
	che chan error
	}

, but found that, it creates a new transaction for every SetAt().

This change is 

[hackerone-code]

✅ A review job has been created and sent to the PullRequest network.

---

@ashish-goswami you can click here to see the review status or cancel the code review job.

[hackerone-code]

	1 Message
✅	The description of this pull request is blank. Adding a high-level summary will help our reviewers provide better feedback.

[hackerone-code]

I don't have any qualms about the code itself, looks good to me. Were there any test cases that should either be modified, or new ones added for this change?

Also, I see the PR is marked as a WIP. I will come back for another round if there are any additional changes.

---

Reviewed with ❤️ by PullRequest

[hackerone-code]

This extra newline doesn't seem necessary

[mangalaman93]

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

---

dgraph/cmd/bulk/reduce.go, line 48 at r1 (raw file):

		}(reduceJob)
	}
	thr.Wait()

Either add a comment that we need to wait for thr before writesThr or increment before go routine is created.

---

dgraph/cmd/bulk/schema.go, line 139 at r1 (raw file):

			x.Check(txn.CommitAt(1, nil))
			txn = db.NewTransactionAt(math.MaxUint64, true)
			x.Check(txn.SetEntry(entry)) // We are not checking ErrTxnTooBig for second time.

add more comments here as to why it doesn't make sense to check ErrTxnTooBig here

[martinmr]

Just a couple minor comments.

For the commit message, make sure you include a short description of the root cause and the solution for future reference.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @mangalaman93, and @manishrjain)

---

dgraph/cmd/bulk/reduce.go, line 93 at r1 (raw file):

			txn = newTxn()
			x.Check(txn.SetEntry(e)) // We are not checking ErrTxnTooBig second time.

nit: "for the second time"

---

dgraph/cmd/bulk/schema.go, line 133 at r1 (raw file):

		// If error returned while setting entry is badger.ErrTxnTooBig, we should
		// commit current txn and start new one.

nit: "a new one"

---

dgraph/cmd/bulk/schema.go, line 139 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

add more comments here as to why it doesn't make sense to check ErrTxnTooBig here

"Not checking for ErrTxnTooBig because transaction was just created" should be enough to clarify the reason.

[ashish-goswami]

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

---

dgraph/cmd/bulk/reduce.go, line 48 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Either add a comment that we need to wait for thr before writesThr or increment before go routine is created.

I have added comment here.

---

dgraph/cmd/bulk/schema.go, line 139 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

"Not checking for ErrTxnTooBig because transaction was just created" should be enough to clarify the reason.

Added detailed comment.

[manishrjain]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ashish-goswami, @mangalaman93, and @martinmr)

---

dgraph/cmd/bulk/schema.go, line 139 at r1 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Added detailed comment.

Why write all this logic. Why not use TxnWriter?

[hackerone-code]

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

[ashish-goswami]

Closing this PR as fixed via #4296