cli: Support nodelocal file upload in CLI #42966

g3orgia · 2019-12-04T22:22:39Z

This change uses the changes made in #42748, which enabled
COPY to upload files. We are now implementing the CLI to include
a new command: nodelocal upload. This command takes in a
source file to upload, and a destination filename. It will
then use the COPY command to upload the file and drop it
on the gateway node's local file system, at:

externalIODir/destination/filename

This PR also includes the CLI testing for this feature as
well as a bug fix for the previous PR #42748:

If writing the file fails, we do not delete the file
that was created. This PR fixes that.

Release note (cli change): Adds a nodelocal command
that can be used to upload file:
cockroach nodelocal upload location/of/file destination/of/file

cockroach-teamcity · 2019-12-04T22:22:47Z

This change is

g3orgia · 2019-12-04T22:25:25Z

There's two things that still need to be changed in this PR:

Verify the version (which is commented out right now). I'm unsure which version I should be verifying. This needs to be changed before this PR gets merged.
Return the nodelocal URI of the file that was written. Is there a way of figuring out the gateway node's nodeID from the CLI? If not, I may need to make a separate PR to pass that information through.

pkg/sql/copy_file_upload_test.go

maddyblue · 2019-12-05T00:04:23Z

pkg/sql/copy_file_upload.go

@@ -112,6 +118,7 @@ func (f *fileUploadMachine) run(ctx context.Context) error {
 	err := f.c.run(ctx)
 	_ = f.writeToFile.Close()
 	if err != nil {
+		f.failureCleanup()


What about if f.writeError has an error? Can/should the API be changed so there's only 1 possible error produced?

I will think about restructuring this. However, since this is on two sides of an io.Pipe, we still need to capture either side's failures.

pkg/sql/conn_executor.go

pkg/cli/nodelocal.go

maddyblue · 2019-12-05T00:12:55Z

pkg/cli/nodelocal.go

+			if err != nil {
+				return err
+			}
+			_, err = stmt.Exec([]driver.Value{string(send[:n])})


can you just exec without casting to a string? that'd save a memory copy.

When I exec as just bytes, it prints the string representation of the bytes to stdin, which gets received that way on the other side. I'm unsure how to receive that on the other side, and convert them back to strings.

Have you tried stmt.Exec(send[:n])? I thought that's how lib/pq's Copy was supposed to work.

Hmm doesn't seem like it. I'll look into how we can structure this better, make use of some of the better suited APIs.

pkg/cli/nodelocal.go

g3orgia · 2019-12-16T19:11:43Z

Two changes:

Outputs nodelocal URL of uploaded file
Error handling using io.Pipe instead of creating our own channel

pkg/sql/copy_file_upload.go

pkg/cli/nodelocal.go

This change uses the changes made in cockroachdb#42748, which enabled COPY to upload files. We are now implementing the CLI to include a new command: `nodelocal upload`. This command takes in a source file to upload, and a destination filename. It will then use the COPY command to upload the file and drop it on the gateway node's local file system, at: `externalIODir/destination/filename` This PR also includes the CLI testing for this feature as well as a bug fix for the previous PR cockroachdb#42748: - If writing the file fails, we do not delete the file that was created. This PR fixes that. Release note (cli change): Adds a nodelocal command that can be used to upload file: `cockroach nodelocal upload location/of/file destination/of/file`

g3orgia · 2019-12-17T17:56:00Z

bors r+

42966: cli: Support nodelocal file upload in CLI r=g3orgia a=g3orgia This change uses the changes made in #42748, which enabled COPY to upload files. We are now implementing the CLI to include a new command: `nodelocal upload`. This command takes in a source file to upload, and a destination filename. It will then use the COPY command to upload the file and drop it on the gateway node's local file system, at: `externalIODir/destination/filename` This PR also includes the CLI testing for this feature as well as a bug fix for the previous PR #42748: - If writing the file fails, we do not delete the file that was created. This PR fixes that. Release note (cli change): Adds a nodelocal command that can be used to upload file: `cockroach nodelocal upload location/of/file destination/of/file` 42973: sql/mutations: stats histograms r=mjibson a=mjibson 43203: sqlmigrations: copy entries into new system.namespace in 20.1 r=arulajmani a=arulajmani In 20.1, CRDB started using a new system.namespace table that had an additional primary key column, `ParentSchemaID`. Previously, all namespace entries created before 20.1 could only be accessed through the old table. This patch migrates them over to the new `system.namespace` table. In the migration: - All databases (`parentID` = 0) are migrated over as is, with the `parentSchemaID` set to 0. - For every database, we add an entry for the `public` schema, which has the ID 29. - All tables (`parentID` != 0) are migrated over as is, with the `parentSchemaID` set to 29. Release note: none Co-authored-by: Georgia Hong <georgiah@cockroachlabs.com> Co-authored-by: Matt Jibson <matt.jibson@gmail.com> Co-authored-by: Arul Ajmani <arulajmani@gmail.com>

craig · 2019-12-17T18:50:40Z

Build succeeded

GitHub CI (Cockroach)

g3orgia requested review from dt and maddyblue December 4, 2019 22:22

g3orgia requested a review from a team as a code owner December 4, 2019 22:22

maddyblue reviewed Dec 5, 2019

View reviewed changes

g3orgia force-pushed the cli-upload branch from 38f0bb8 to ae2b0b6 Compare December 5, 2019 15:48

maddyblue reviewed Dec 5, 2019

View reviewed changes

pkg/cli/nodelocal.go Outdated Show resolved Hide resolved

g3orgia force-pushed the cli-upload branch from ae2b0b6 to f777727 Compare December 6, 2019 18:53

maddyblue approved these changes Dec 12, 2019

View reviewed changes

g3orgia force-pushed the cli-upload branch from f777727 to a6b5309 Compare December 16, 2019 19:09

g3orgia force-pushed the cli-upload branch from a6b5309 to 19a8ce7 Compare December 16, 2019 19:22

maddyblue reviewed Dec 16, 2019

View reviewed changes

pkg/sql/copy_file_upload.go Outdated Show resolved Hide resolved

pkg/sql/copy_file_upload.go Outdated Show resolved Hide resolved

maddyblue reviewed Dec 16, 2019

View reviewed changes

pkg/cli/nodelocal.go Outdated Show resolved Hide resolved

g3orgia force-pushed the cli-upload branch from 19a8ce7 to 6d6b38c Compare December 16, 2019 21:57

maddyblue approved these changes Dec 16, 2019

View reviewed changes

g3orgia force-pushed the cli-upload branch from 6d6b38c to c461361 Compare December 17, 2019 16:10

g3orgia force-pushed the cli-upload branch from c461361 to 488661c Compare December 17, 2019 16:33

craig bot merged commit 488661c into cockroachdb:master Dec 17, 2019

g3orgia deleted the cli-upload branch December 17, 2019 18:53

jseldess mentioned this pull request Feb 5, 2020

Enable single-node nodelocal:// storage cockroachdb/docs#6010

Closed

knz added this to To do in DB Server & Security via automation Mar 7, 2020

knz moved this from To do to Done 20.1 in DB Server & Security Mar 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: Support nodelocal file upload in CLI #42966

cli: Support nodelocal file upload in CLI #42966

g3orgia commented Dec 4, 2019

cockroach-teamcity commented Dec 4, 2019

g3orgia commented Dec 4, 2019

maddyblue Dec 5, 2019

g3orgia Dec 5, 2019

maddyblue Dec 5, 2019

g3orgia Dec 5, 2019

maddyblue Dec 5, 2019

g3orgia Dec 6, 2019

g3orgia commented Dec 16, 2019

g3orgia commented Dec 17, 2019

craig bot commented Dec 17, 2019

cli: Support nodelocal file upload in CLI #42966

cli: Support nodelocal file upload in CLI #42966

Conversation

g3orgia commented Dec 4, 2019

cockroach-teamcity commented Dec 4, 2019

g3orgia commented Dec 4, 2019

maddyblue Dec 5, 2019

Choose a reason for hiding this comment

g3orgia Dec 5, 2019

Choose a reason for hiding this comment

maddyblue Dec 5, 2019

Choose a reason for hiding this comment

g3orgia Dec 5, 2019

Choose a reason for hiding this comment

maddyblue Dec 5, 2019

Choose a reason for hiding this comment

g3orgia Dec 6, 2019

Choose a reason for hiding this comment

g3orgia commented Dec 16, 2019

g3orgia commented Dec 17, 2019

craig bot commented Dec 17, 2019

Build succeeded