Add bulk loader validator #3838

animesh2049 · 2019-08-20T12:04:23Z

bulk validator reports all the parsing error in input files

This change is

Updates #3984

bulk validator reports all the parsing error in input files

dgraph/cmd/bulk_validator/loader.go

This tool can also be used to validate input files for live loader.

pullrequest

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

@animesh2049 you can click here to see the review status or cancel the code review job.

pullrequest

Left some inline feedback regarding error handling improvements and consistency.

Reviewed with ❤️ by PullRequest

dgraph/cmd/validator/loader.go

dgraph/cmd/validator/run.go

dgraph/cmd/validator/loader.go

dgraph/cmd/root.go

dgraph/cmd/validator/loader.go

dgraph/cmd/validator/mapper.go

gitlw

It seems like all the logic contained in this PR is also available in the current bulk loader. Is there anything new that this validator provides?

Reviewable status: 0 of 4 files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @manishrjain)

animesh2049 · 2019-09-02T07:55:27Z

It seems like all the logic contained in this PR is also available in the current bulk loader. Is there anything new that this validator provides?

Reviewable status: 0 of 4 files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @manishrjain)

Yes all the logic is already there in bulk loader but since we want a different validator tool, we thought it would be good to do it this way. What are your thoughts on this @gitlw ?

gitlw

I had thought Manish wanted a more rigorous validation tool. So maybe check with him first. Thanks!

Reviewable status: 0 of 4 files reviewed, 9 unresolved discussions (waiting on @animesh2049 and @manishrjain)

pullrequest

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

mangalaman93

We should add test in systest

mangalaman93 · 2019-08-22T09:33:21Z

dgraph/cmd/bulk_validator/loader.go

@@ -0,0 +1,122 @@
+package bulkvalidator


Could just call if validator instead of bulk_validator. This could be use for live loader data as well.

mangalaman93 · 2019-08-22T09:33:39Z

dgraph/cmd/bulk_validator/loader.go

+
+	"github.com/dgraph-io/badger/y"
+	"github.com/dgraph-io/dgraph/chunker"
+


remove newline here

mangalaman93 · 2019-09-06T09:31:24Z

dgraph/cmd/validator/loader.go

+	"github.com/dgraph-io/dgraph/chunker"
+	"github.com/prometheus/common/log"
+
+	"github.com/dgraph-io/dgraph/x"


Move this import together with other import.

mangalaman93 · 2019-09-09T09:30:53Z

dgraph/cmd/validator/loader.go

+		x.Fatalf("No data files found in %s\n", ld.opt.DataFiles)
+	}
+
+	loadType := chunker.DataFormat(files[0], ld.opt.DataFormat)


Should we check here that all the files have correct type?

mangalaman93 · 2019-09-09T09:31:41Z

dgraph/cmd/validator/loader.go

+				if err == io.EOF {
+					break
+				} else if err != nil {
+					x.Check(err)


Will it make sense to continue here instead?

mangalaman93 · 2019-09-09T09:33:37Z

dgraph/cmd/validator/mapper.go

+
+		for chunkBuf := range m.readerChunkCh {
+			if err := chunker.Parse(chunkBuf.chunk); err != nil {
+				m.foundError = true


Are we syncronizing access to foundError?

animesh2049

Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @golangcibot, @mangalaman93, @manishrjain, and @pullrequest[bot])

dgraph/cmd/validator/mapper.go, line 29 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Similar to other comment regarding fmt vs log would potentially switch this to use log since its being used concurrently

Done.

dgraph/cmd/validator/mapper.go, line 28 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Are we syncronizing access to foundError?

Done.

dgraph/cmd/bulk_validator/loader.go, line 56 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Println call has possible formatting directive %s (from govet)

Done.

dgraph/cmd/validator/loader.go, line 61 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Since some of the errors are being handled with x.Check which appears to use log.Fatal, would it make sense to just use log.Fatalln instead of fmt.Println with an os.Exit just so that all of the error output is using log?

Done.

dgraph/cmd/validator/loader.go, line 68 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Could possibly add a newline onto the end of this printf to match the others

Done.

dgraph/cmd/validator/loader.go, line 75 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Might simplify this to do:
go m.run(loadType, &mapperWg)
Although it actually looks like run() does everything except initializing the chunker on a separate goroutine so it may not be necessary to do this on another goroutine at all?

Done.

dgraph/cmd/validator/loader.go, line 86 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Using log here as well might be better since this is being done concurrently as the log package locks around writing to std output, where fmt doesn't.

Done.

dgraph/cmd/validator/loader.go, line 92 at r2 (raw file):

Previously, pullrequest[bot] wrote…

It looks like x.Check does a log.Fatal which I believe will actually exit without a panic so this will skip your deferred functions above. Just wanted to check if that is okay? There is another usage below as well.

Defer function cleanup() only closes the file. log.Fatal should kill the program and hence close the file also.

dgraph/cmd/validator/loader.go, line 12 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Move this import together with other import.

Done.

dgraph/cmd/validator/loader.go, line 63 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Should we check here that all the files have correct type?

It's just taking file type from the first input file. If all the files are not of the same type, chunker will give error while chunking.

dgraph/cmd/validator/loader.go, line 99 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Will it make sense to continue here instead?

I don't think so, because this error is likely to come in the case of file format mismatch i.e. chunker expects different format.

mangalaman93

Reviewed 1 of 4 files at r3.
Reviewable status: 1 of 4 files reviewed, 12 unresolved discussions (waiting on @animesh2049, @golangcibot, @manishrjain, and @pullrequest[bot])

dgraph/cmd/validator/mapper.go, line 29 at r4 (raw file):

		for chunkBuf := range m.readerChunkCh {
			if err := chunker.Parse(chunkBuf.chunk); err != nil {
				atomic.CompareAndSwapUint32(&m.foundError, 0, 1)

I think it'd be okay to not print anything in case there is no error.

dgraph/cmd/validator/mapper.go, line 30 at r4 (raw file):

			if err := chunker.Parse(chunkBuf.chunk); err != nil {
				atomic.CompareAndSwapUint32(&m.foundError, 0, 1)
				glog.Errorf("Error Found in file %s: %s\n", chunkBuf.filename, err)

found*

dgraph/cmd/validator/mapper.go, line 32 at r4 (raw file):

				glog.Errorf("Error Found in file %s: %s\n", chunkBuf.filename, err)
			}

newline

dgraph/cmd/validator/mapper.go, line 36 at r4 (raw file):

	}()

	go func() {

comment

dgraph/cmd/validator/loader.go, line 68 at r2 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Done.

same here

dgraph/cmd/validator/run.go, line 59 at r4 (raw file):

	}

	loader := newLoader(opt)

no need for assignment.

manishrjain

This PR is unnecessarily complex. Instead of trying to copy the bulk loader structure, think about what is the minimum you need to make things work nicely. Needs refactoring.

Reviewable status: 1 of 4 files reviewed, 19 unresolved discussions (waiting on @animesh2049, @golangcibot, @manishrjain, and @pullrequest[bot])

dgraph/cmd/validator/mapper.go, line 1 at r5 (raw file):

package validator

License?

dgraph/cmd/validator/mapper.go, line 11 at r5 (raw file):

)

type mapper struct {

You don't need this object.

dgraph/cmd/validator/mapper.go, line 21 at r5 (raw file):

}

func (m *mapper) run(inputFormat chunker.InputFormat, wg *sync.WaitGroup) {

You only need this function.

dgraph/cmd/validator/loader.go, line 1 at r5 (raw file):

package validator

License?

dgraph/cmd/validator/loader.go, line 18 at r5 (raw file):

	TmpDir        string
	NumGoroutines int
	CleanupTmp    bool

Do you need these options?

dgraph/cmd/validator/loader.go, line 33 at r5 (raw file):

}

type loader struct {

Probably don't need the loader object either.

dgraph/cmd/validator/loader.go, line 35 at r5 (raw file):

type loader struct {
	*state
	mappers []*mapper

Don't need these mapper objects.

dgraph/cmd/validator/loader.go, line 88 at r5 (raw file):

			for {
				chunkBuf, err := chunker.Chunk(r)
				if chunkBuf != nil && chunkBuf.Len() > 0 {

This should happen after err has been handled, right?

dgraph/cmd/validator/run.go, line 1 at r5 (raw file):

package validator

License?

dgraph/cmd/validator/run.go, line 20 at r5 (raw file):

func init() {
	Validator.Cmd = &cobra.Command{
		Use:   "validator",

validate

dgraph/cmd/validator/run.go, line 21 at r5 (raw file):

	Validator.Cmd = &cobra.Command{
		Use:   "validator",
		Short: "Validate input file",

files

dgraph/cmd/validator/run.go, line 60 at r5 (raw file):

	loader := newLoader(opt)
	loader.mapStage()

Add a message at the end saying the validation is done or something.

Add bulk validator

72efbff

bulk validator reports all the parsing error in input files

animesh2049 requested a review from manishrjain as a code owner August 20, 2019 12:04

animesh2049 requested a review from a team August 20, 2019 12:04

golangcibot reviewed Aug 20, 2019

View reviewed changes

dgraph/cmd/bulk_validator/loader.go Outdated Show resolved Hide resolved

animesh2049 added 2 commits August 20, 2019 21:34

Remove deadlock on chunkbuf channel

41ab9a1

Rename from bulk_validator to validator

d683890

This tool can also be used to validate input files for live loader.

animesh2049 changed the title ~~[WIP] Add bulk loader validator~~ Add bulk loader validator Aug 22, 2019

pullrequest bot reviewed Aug 22, 2019

View reviewed changes

dgraph/cmd/validator/mapper.go Outdated Show resolved Hide resolved

Address PR comments

0c63ea8

gitlw reviewed Aug 29, 2019

View reviewed changes

gitlw reviewed Sep 3, 2019

View reviewed changes

pullrequest bot reviewed Sep 6, 2019

View reviewed changes

mangalaman93 requested changes Sep 9, 2019

View reviewed changes

animesh2049 added 2 commits September 10, 2019 16:21

Synchronize foundError and use glog

10c3446

Remove schema flag

89511b0

animesh2049 commented Sep 10, 2019

View reviewed changes

mangalaman93 approved these changes Sep 10, 2019

View reviewed changes

animesh2049 added 2 commits September 10, 2019 17:37

Modify error message and add comment

a0cefdf

Address PR comments

f6eca59

manishrjain suggested changes Sep 17, 2019

View reviewed changes

campoy mentioned this pull request Sep 17, 2019

Generate blank UIDs based on JSON fields on mutations and loaders #2848

Closed

animesh2049 added 3 commits September 26, 2019 17:43

Minimal implementation of validator

dd95df7

Add license file

efe3cf6

Address previous commments

205f749

animesh2049 closed this Jan 14, 2020

animesh2049 deleted the animesh2049/bulk_loader_file_validator branch January 14, 2020 14:33


		"github.com/dgraph-io/badger/y"
		"github.com/dgraph-io/dgraph/chunker"

Add bulk loader validator #3838

Add bulk loader validator #3838

Uh oh!

Conversation

animesh2049 commented Aug 20, 2019 • edited by campoy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitlw left a comment

Choose a reason for hiding this comment

Uh oh!

animesh2049 commented Sep 2, 2019

Uh oh!

gitlw left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

mangalaman93 left a comment

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Sep 6, 2019

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

mangalaman93 Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

animesh2049 left a comment

Choose a reason for hiding this comment

Uh oh!

mangalaman93 left a comment

Choose a reason for hiding this comment

Uh oh!

manishrjain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

animesh2049 commented Aug 20, 2019 •

edited by campoy

Loading