-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remotetool: add command to upload a tree into the CAS #287
base: master
Are you sure you want to change the base?
Conversation
if _, _, err := c.GrpcClient.UploadIfMissing(ctx, ue); err != nil { | ||
// UploadTree uploads a tree from the specified path into the remote cache. | ||
func (c *Client) UploadTree(ctx context.Context, concurrency uint64, path string) error { | ||
uploader := newParallelUploader(ctx, c.GrpcClient, concurrency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why couldn't you use UploadIfMissing
directly instead of making a parallel uploaded? UploadIfMissing
should already has parallel support, controlled by CASConcurrency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using this tool to upload very large trees with 10,000s files. Just iterating over all of them takes significant time, so it's more efficient to start uploading while iterating over the files in the tree.
I'll have a look if I can achieve this with the existing parallel uploading.
execAttempts = flag.Int("exec_attempts", 10, "For check_determinism: the number of times to remotely execute the action and check for mismatches.") | ||
operation = flag.String("operation", "", fmt.Sprintf("Specifies the operation to perform. Supported values: %v", supportedOps)) | ||
digest = flag.String("digest", "", "Digest in <digest/size_bytes> format.") | ||
pathPrefix = flag.String("path", "", "Path to which outputs should be downloaded to.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this documentation should be updated?
if _, _, err := c.GrpcClient.UploadIfMissing(ctx, ue); err != nil { | ||
// UploadTree uploads a tree from the specified path into the remote cache. | ||
func (c *Client) UploadTree(ctx context.Context, concurrency uint64, path string) error { | ||
uploader := newParallelUploader(ctx, c.GrpcClient, concurrency) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this.
if err := toolClient.UploadTree(context.Background(), tempDir); err != nil { | ||
t.Fatalf("UploadTree('%v') failed: %v", tmpFile, err) | ||
} | ||
if cas.WriteReqs() != 4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are skipping the blob upload during second call to the same tempDir
path, then shouldn't we have 0 write requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stats aren't reset, so they continue to be 4 from the initial upload. If the second call uploads anything we'd get > 4 here.
Was this superseded by remote-apis-sdks/go/pkg/tool/tool.go Line 341 in e74bc3d
|
No description provided.