lakectl fs use sync manger #6650
Conversation
idanovo
added
area/lakectl
| ) | ||
|
|
||
| var fsDownloadCmd = &cobra.Command{ | ||
| Use: "download <path uri> [<destination path>]", | ||
| Short: "Download object(s) from a given repository path", | ||
| Args: cobra.RangeArgs(fsDownloadCmdMinArgs, fsDownloadCmdMaxArgs), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| pathURI := MustParsePathURI("path", args[0]) | ||
| remote, dest := getLocalArgs(args, true, false) | ||
| flagSet := cmd.Flags() | ||
| preSignMode := Must(flagSet.GetBool("pre-sign")) |
There was a problem hiding this comment.
You can use getLocalSyncFlags for the presign and parallelism flags (Though I would remove the Local from its name)
There was a problem hiding this comment.
can't use it as it uses pre-signed as true by default
There was a problem hiding this comment.
We might want to change that to be true by default, maybe @ozkatz can comment on that
There was a problem hiding this comment.
I think it will be a better thing to do it in a different PR especially because this issue
There was a problem hiding this comment.
Overall looks good.
I'd like to raise 2 fundamental issues which might require a product discussion:
- We might want to consider consolidating the single / recursive flows and use the sync manager in both + removing the recursive flag
- We should take advantage of the parallelism of the sync manager and introduce this parameter in the upload command + use the default value
- The sync manager provides a summary of the performed operations - we should use this as the output for the commands (you can see examples of usage in the local commands)
cmd/lakectl/cmd/fs_download.go
Outdated
| ) | ||
|
|
||
| var fsDownloadCmd = &cobra.Command{ | ||
| Use: "download <path uri> [<destination path>]", | ||
| Short: "Download object(s) from a given repository path", | ||
| Args: cobra.RangeArgs(fsDownloadCmdMinArgs, fsDownloadCmdMaxArgs), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| pathURI := MustParsePathURI("path", args[0]) | ||
| remote, dest := getLocalArgs(args, true, false) | ||
| flagSet := cmd.Flags() | ||
| preSignMode := Must(flagSet.GetBool("pre-sign")) | ||
| recursive := Must(flagSet.GetBool("recursive")) |
There was a problem hiding this comment.
Also we might want to move the parallel check ( <1 ) to getLocalSyncFlags
cmd/lakectl/cmd/fs_download.go
Outdated
| for i := 0; i < parallel; i++ { | ||
| s := local.NewSyncManager(ctx, client, parallel, preSignMode) | ||
| remotePath := remote.GetPath() | ||
| if recursive { |
There was a problem hiding this comment.
Valid flow also for !recursive
We should probably eliminate this param
pkg/local/sync.go
Outdated
| @@ -129,18 +129,18 @@ func (s *SyncManager) apply(ctx context.Context, rootPath string, remote *uri.UR | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *SyncManager) download(ctx context.Context, rootPath string, remote *uri.URI, change *Change) error { | |||
| if err := fileutil.VerifyRelPath(strings.TrimPrefix(change.Path, uri.PathSeparator), rootPath); err != nil { | |||
| func (s *SyncManager) Download(ctx context.Context, rootPath string, remote *uri.URI, path string) error { | |||
There was a problem hiding this comment.
Not we really need to externalize the Download and Upload methods
pkg/local/sync.go
Outdated
| return fmt.Errorf("download %s failed: %w", change.Path, err) | ||
| } | ||
| case ChangeSourceLocal: | ||
| // we wrote something, upload it! | ||
| if err := s.upload(ctx, rootPath, remote, change); err != nil { | ||
| // we wrote something, Upload it! |
pkg/local/sync.go
Outdated
| @@ -98,12 +98,12 @@ func (s *SyncManager) apply(ctx context.Context, rootPath string, remote *uri.UR | |||
| switch change.Source { | |||
| case ChangeSourceRemote: | |||
| // remotely changed something, download it! | |||
| if err := s.download(ctx, rootPath, remote, change); err != nil { | |||
| if err := s.Download(ctx, rootPath, remote, change.Path); err != nil { | |||
|
♻️ PR Preview 0082297 has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
Closes #6321