driver and native driver performance testing commands + refactor

[lwsanty]

closes #21

Signed-off-by: lwsanty lwsanty@gmail.com

---

This change is 

[dennwc]

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @lwsanty)

---

go.mod, line 11 at r1 (raw file):

	github.com/cenkalti/backoff v2.1.1+incompatible // indirect
	github.com/creachadair/staticfile v0.0.3
	github.com/lib/pq v1.2.0 // indirect

Why it's needed?

---

cmd/bblfsh-performance/driver/driver.go, line 112 at r1 (raw file):

	flags.String("filter-prefix", fileFilterPrefix, "file prefix to be filtered")
	flags.StringP("storage", "s", prom.Kind, "storage kind to store the results"+
		fmt.Sprintf("(%s, %s, %s)", prom.Kind, influxdb.Kind, file.Kind))

You can add a description text to Sprintf as well.

---

cmd/bblfsh-performance/drivernative/drivernative.go, line 103 at r1 (raw file):

				case <-c:
					cancel()
					os.Exit(1)

Looks like a hack. Why do you need this here?

---

cmd/bblfsh-performance/drivernative/drivernative.go, line 162 at r1 (raw file):

	flags.String("filter-prefix", fileFilterPrefix, "file prefix to be filtered")
	flags.StringP("storage", "s", prom_pushgateway.Kind, "storage kind to store the results"+
		fmt.Sprintf("(%s, %s, %s)", prom_pushgateway.Kind, influxdb.Kind, file.Kind))

same for the description as in the other command

---

cmd/native-driver-performance/main.go, line 25 at r1 (raw file):

	fixtures := flag.String("fixtures", "", "path to fixtures directory")
	resultsFile := flag.String("results", "", "path to file to store benchmark results")
	filterPrefix := flag.String("filter-prefix", "", "file")

unfinished flag description?

---

cmd/native-driver-performance/main.go, line 39 at r1 (raw file):

	if err != nil {
		log.Infof("cannot get files: %v", err)
		os.Exit(1)

I propose to add a func run() error and place all the code there. This way main can run the function, check error and Exit if necessary (in a single place).

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

	defer func() {
		if err := f.Close(); err != nil {
			log.Infof("failed to close file %v: %v", file, err)

should Exit(1) as well? same for Write below

---

docker/docker.go, line 61 at r1 (raw file):

	defer f.Close()

	sh := fmt.Sprintf("cat > %[1]s ; chmod +x %[1]s", dst)

That's an interesting way of uploading files. Will it work for binary files? There should be a better way to copy files to the container.

---

docker/docker.go, line 78 at r1 (raw file):

		Context:      ctx,
		InputStream:  f,
		OutputStream: os.Stdout,

I propose to redirect stdout to os.Stderr as well, or it may interfere with CLI output.

---

docker/docker.go, line 105 at r1 (raw file):

	var buf bytes.Buffer
	w := bufio.NewWriter(&buf)

You don't need budio.Writer here. Just pass &buf to stdout.

---

docker/docker.go, line 146 at r1 (raw file):

		RawTerminal:  true,
		Tty:          true,
		InputStream:  os.Stdin,

Do we need to redirect stdin? And the same suggestion to stdout as above.

---

docker/docker.go, line 156 at r1 (raw file):

	inspect, err := d.Pool.Client.InspectExec(exec.ID)
	if err != nil {
		return errExecFailed.Wrap(err)

Should it kill/remove exec?

---

docker/docker.go, line 238 at r1 (raw file):

	log.Debugf("waiting for port")
	if err := wait(pool, addr); err != nil {
		return nil, err

Should it remove the resource?

[lwsanty]

Reviewable status: 10 of 14 files reviewed, 13 unresolved discussions (waiting on @dennwc)

---

go.mod, line 11 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Why it's needed?

that's weird, go mod tidy did not remove it, I removed that deps manually and tried to rebuild binaries, seems to be working o_0

---

cmd/bblfsh-performance/driver/driver.go, line 112 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

You can add a description text to Sprintf as well.

Done.

---

cmd/bblfsh-performance/drivernative/drivernative.go, line 103 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Looks like a hack. Why do you need this here?

It looks like I had some lock during parse request or so, so I could not terminate the process, anyways removed because cannot reproduce it now
Done.

---

cmd/bblfsh-performance/drivernative/drivernative.go, line 162 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

same for the description as in the other command

Done.

---

cmd/native-driver-performance/main.go, line 25 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

unfinished flag description?

Done.

---

cmd/native-driver-performance/main.go, line 39 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I propose to add a func run() error and place all the code there. This way main can run the function, check error and Exit if necessary (in a single place).

Done.

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

should Exit(1) as well? same for Write below

Done for Write, but I've got a little confusion about close. Is it crucial to fail all the process if close has failed?

---

docker/docker.go, line 61 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

That's an interesting way of uploading files. Will it work for binary files? There should be a better way to copy files to the container.

It works for binary files.
I agree that it looks hackish.

As alternative I considered UploadToContainer method from API but it deals with tar archives and IMO it's an overkill for our task, I can leave TODO to refactor this part in case we will have a need to upload some more various data to container, but in this case we can always pass volume instead of archive.

About another direction, there was method CopyFromContainer but unfortunately it's deprecated.

---

docker/docker.go, line 78 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I propose to redirect stdout to os.Stderr as well, or it may interfere with CLI output.

Done.

---

docker/docker.go, line 105 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

You don't need budio.Writer here. Just pass &buf to stdout.

Done.

---

docker/docker.go, line 146 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Do we need to redirect stdin? And the same suggestion to stdout as above.

I guess we do not deal with stdin here yet, removed

---

docker/docker.go, line 156 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Should it kill/remove exec?

I think no, because:

• I can't see such option in api
• we will fail the test and remove container anyways

am I missing smth?

---

docker/docker.go, line 238 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Should it remove the resource?

Done.

[dennwc]

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lwsanty)

---

go.mod, line 11 at r1 (raw file):

Previously, lwsanty wrote…

that's weird, go mod tidy did not remove it, I removed that deps manually and tried to rebuild binaries, seems to be working o_0

Check if you have other uncomitted files locally. go mod doesn't use Git, so it will list all the deps from all Go files in the current project.

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, lwsanty wrote…

Done for Write, but I've got a little confusion about close. Is it crucial to fail all the process if close has failed?

Yes. It usually means the Flush on file failed.

For the context, there is a notion that "writes never fail, but flush/fsync may". The reason is that OS buffers most writes to memory and flushes them to disk when the files is closed.

---

cmd/native-driver-performance/main.go, line 78 at r2 (raw file):

	data, err := json.Marshal(benchmarks)
	if err != nil {
		return errors.NewKind("failed to marshal results").Wrap(err)

errors.New may be better here (stdlib's errors, not src-d/errors)

---

docker/docker.go, line 61 at r1 (raw file):

Previously, lwsanty wrote…

It works for binary files.
I agree that it looks hackish.

As alternative I considered UploadToContainer method from API but it deals with tar archives and IMO it's an overkill for our task, I can leave TODO to refactor this part in case we will have a need to upload some more various data to container, but in this case we can always pass volume instead of archive.

About another direction, there was method CopyFromContainer but unfortunately it's deprecated.

TODO sounds fine. Note that Go has a archive/tar package that may solve the issues with uploading a tar archive.

---

docker/docker.go, line 156 at r1 (raw file):

Previously, lwsanty wrote…

I think no, because:

• I can't see such option in api
• we will fail the test and remove container anyways

am I missing smth?

OK, was not sure if such API exists either, but wanted to make sure.

[lwsanty]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dennwc)

---

go.mod, line 11 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Check if you have other uncomitted files locally. go mod doesn't use Git, so it will list all the deps from all Go files in the current project.

Don't have any, probably I had one for debug before, but now - nope

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Yes. It usually means the Flush on file failed.

For the context, there is a notion that "writes never fail, but flush/fsync may". The reason is that OS buffers most writes to memory and flushes them to disk when the files is closed.

Done

---

cmd/native-driver-performance/main.go, line 78 at r2 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

errors.New may be better here (stdlib's errors, not src-d/errors)

I think it does not worth import stdlib's errors just for this line of code, or you mean everywhere in the file?

---

docker/docker.go, line 61 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

TODO sounds fine. Note that Go has a archive/tar package that may solve the issues with uploading a tar archive.

Done.

[dennwc]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lwsanty)

---

go.mod, line 11 at r1 (raw file):

Previously, lwsanty wrote…

Don't have any, probably I had one for debug before, but now - nope

We need to find out why it adds this dependency. I don't think we should depend on it.

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, lwsanty wrote…

Done

I don't see any changes here.

---

cmd/native-driver-performance/main.go, line 78 at r2 (raw file):

Previously, lwsanty wrote…

I think it does not worth import stdlib's errors just for this line of code, or you mean everywhere in the file?

Yeah, every line that does errors.NewKind(...).Wrap(err). And sorry, I've meant fmt.Errorf instead of errors here.

---

docker/docker.go, line 61 at r1 (raw file):

Previously, lwsanty wrote…

Done.

Again, don't see any changes. Have you pushed it to the remote?

[lwsanty]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dennwc)

---

go.mod, line 11 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

We need to find out why it adds this dependency. I don't think we should depend on it.

.../go/pkg/mod/github.com/ory/dockertest@v3.3.4+incompatible/dockertest_test.go
probably the example with dockertest that runs postgres container

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I don't see any changes here.

https://github.com/bblfsh/performance/pull/22/files
changes are visible for me, for example this one
I return both errors to not shadow the original one

defer func() {
		if closeErr := f.Close(); closeErr != nil {
			log.Infof("failed to close file %v: %v", file, closeErr)
			err = fmt.Errorf("err: %v, closeErr: %v", err, closeErr)
		}
	}()

---

cmd/native-driver-performance/main.go, line 78 at r2 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Yeah, every line that does errors.NewKind(...).Wrap(err). And sorry, I've meant fmt.Errorf instead of errors here.

Done.

---

docker/docker.go, line 61 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Again, don't see any changes. Have you pushed it to the remote?

Again, https://github.com/bblfsh/performance/pull/22/files

// TODO(lwsanty): use UploadToContainer for more various data
// Upload uploads given file from host to container
// source file's descriptor is used as input stream
// docker executes cat command to redirect this stream to the destination file
func (d *Driver) Upload(ctx context.Context, src, dst string) error {

[dennwc]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lwsanty)

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, lwsanty wrote…

https://github.com/bblfsh/performance/pull/22/files
changes are visible for me, for example this one
I return both errors to not shadow the original one

defer func() {
		if closeErr := f.Close(); closeErr != nil {
			log.Infof("failed to close file %v: %v", file, closeErr)
			err = fmt.Errorf("err: %v, closeErr: %v", err, closeErr)
		}
	}()

I would suggest to call the returned error gerr (or any other name except err). It would be more clear when you override the returned value instead of a local function variable.

---

docker/docker.go, line 61 at r1 (raw file):

Previously, lwsanty wrote…

Again, https://github.com/bblfsh/performance/pull/22/files

// TODO(lwsanty): use UploadToContainer for more various data
// Upload uploads given file from host to container
// source file's descriptor is used as input stream
// docker executes cat command to redirect this stream to the destination file
func (d *Driver) Upload(ctx context.Context, src, dst string) error {

Sorry, it was a Reviewable failure :(

[lwsanty]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dennwc)

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I would suggest to call the returned error gerr (or any other name except err). It would be more clear when you override the returned value instead of a local function variable.

Done

[dennwc]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lwsanty)

---

cmd/native-driver-performance/main.go, line 86 at r1 (raw file):

Previously, lwsanty wrote…

Done

Now every error variable in the function is called gerr. This is not what I meant. Now you'll need to pick other name instead of gerr since the code still uses the same error variable for both the return and for local usage 😅

Please refactor it back to err, then manually rename return to gerr and set it in defer only.

[dennwc]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved