Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514343005:
client/isolate: add skeleton exparchive command (Closed)
|
Created:
4 years, 1 month ago by djd-OOO-Apr2017 Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Descriptionclient/isolate: add skeleton exparchive command
This experimental archive command will support archiving smaller files
into large batches.
It is being developed separately from the existing archive/batcharchive
command to allow more freedom to explore the space and to investigate
simpler pipelining of steps.
Issue luci/luci-go#9
BUG=59899
R=mcgreevy@chromium.org, tansell@chromium.org
Committed: https://github.com/luci/luci-go/commit/74934dfa148729458ce3adf87904733c6e46cfe5
Patch Set 1 #
Total comments: 8
Patch Set 2 : mcgreevy fixes, tighten signatures #
Total comments: 8
Patch Set 3 : fix flag handling a bit more #
Total comments: 1
Patch Set 4 : mcgreevy comments #
Dependent Patchsets: Messages
Total messages: 15 (3 generated)
This is the just the start of the exp archive command Tim and I have been working on. The structure here is stolen directly from the existing archive command.
https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:23: ShortDesc: "EXPERIMENTAL creates a .isolated file and uploads the tree to an isolate server.", "the tree" can you be more explicit? what tree? https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:33: type expArchiveRun struct { Can you please document this? https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:34: commonServerFlags Is there any reason for these to be anonymous fields? I generally try to avoid them unless I want methods on the anonymous fields to be promoted. https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:46: if err := c.isolateFlags.Parse(cwd, RequireIsolatedFile); err != nil { Don't you need "RequireIsolateFile&RequireIsolatedFile" here? I think that line 57 assumes that c.ArchiveOptions contains an isolate file.
https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:23: ShortDesc: "EXPERIMENTAL creates a .isolated file and uploads the tree to an isolate server.", On 2016/11/22 at 05:12:12, mcgreevy2 wrote: > "the tree" can you be more explicit? what tree? Tried to add a lot more explanatory text, PTAL. https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:33: type expArchiveRun struct { On 2016/11/22 at 05:12:12, mcgreevy2 wrote: > Can you please document this? Done. https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:34: commonServerFlags On 2016/11/22 at 05:12:12, mcgreevy2 wrote: > Is there any reason for these to be anonymous fields? I generally try to avoid them unless I want methods on the anonymous fields to be promoted. commonServerFlags needed to satisfy the subcommand.CommandRun interface. I've named the isolateFlags field now (the only reason I could see to make it anon. was to shorten the references to ArchiveOptions below). https://codereview.chromium.org/2514343005/diff/1/client/cmd/isolate/exp_arch... client/cmd/isolate/exp_archive.go:46: if err := c.isolateFlags.Parse(cwd, RequireIsolatedFile); err != nil { On 2016/11/22 at 05:12:12, mcgreevy2 wrote: > Don't you need "RequireIsolateFile&RequireIsolatedFile" here? I think that line 57 assumes that c.ArchiveOptions contains an isolate file. Good call, fixed.
https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:23: ShortDesc: "EXPERIMENTAL parses a .isolate file to creates a .isolated file, and uploads it and all referenced files to an isolate server", s/to creates/to create/ https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:43: deps, rootDir, isol, err := isolate.ProcessIsolate(&c.isolateFlags.ArchiveOptions) before this, can you do: archiveOpts := &c.isolateFlags.ArchiveOptions and use throughout? It's be nice if we didn't hang on to isolateFlags at all -- just stored ArchiveOptions, but as written it needs to be a field of expArchiveRun so that it can be initialized on line 28, and then parsed on line 76. Could you just create and Init isolateFlags as a temporary before line 76, and then return ArchiveOptions from parseFlags, then pass it as an argument to main (maybe call main something else)?
https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:68: func (c *expArchiveRun) parseFlags(a subcommands.Application, args []string) error { The "a" parameter does not seem to be used. https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:79: if len(args) != 0 { This seems like something you could do up-front before doing all of the other parsing work.
https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:23: ShortDesc: "EXPERIMENTAL parses a .isolate file to creates a .isolated file, and uploads it and all referenced files to an isolate server", On 2016/11/22 at 23:43:35, mcgreevy2 wrote: > s/to creates/to create/ Done. https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:43: deps, rootDir, isol, err := isolate.ProcessIsolate(&c.isolateFlags.ArchiveOptions) On 2016/11/22 at 23:43:35, mcgreevy2 wrote: > before this, can you do: > > archiveOpts := &c.isolateFlags.ArchiveOptions > > and use throughout? > > It's be nice if we didn't hang on to isolateFlags at all -- just stored ArchiveOptions, but as written it needs to be a field of expArchiveRun so that it can be initialized on line 28, and then parsed on line 76. > > Could you just create and Init isolateFlags as a temporary before line 76, and then return ArchiveOptions from parseFlags, then pass it as an argument to main (maybe call main something else)? I would like to be able to do that, but I don't think I can: the initialisation of the flags needs to happen before we return the CommandRun. This is because GetFlags needs the full list of flags to support help (./isolate help exparchive). If I do it only in Main then that help text is missed. I agree this is gross as it stands, but making it more einsteinian will invole multiple package's clean up. (tandrii already has a cleanup TODO for the isolateFlags struct too – so I guess this is a known smell) PTAL what I did to try to make this a bit friendlier. https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:68: func (c *expArchiveRun) parseFlags(a subcommands.Application, args []string) error { On 2016/11/22 at 23:54:38, mcgreevy2 wrote: > The "a" parameter does not seem to be used. Done. https://codereview.chromium.org/2514343005/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:79: if len(args) != 0 { On 2016/11/22 at 23:54:38, mcgreevy2 wrote: > This seems like something you could do up-front before doing all of the other parsing work. Done.
https://codereview.chromium.org/2514343005/diff/40001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2514343005/diff/40001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:39: archiveOpts *isolate.ArchiveOptions As discussed, when reading this it takes extra effort to verify that archiveOpts just points to a field within isolateFlags. I think pulling it out at the top of main is clearer.
On 2016/11/23 at 00:25:22, mcgreevy wrote: > https://codereview.chromium.org/2514343005/diff/40001/client/cmd/isolate/exp_... > File client/cmd/isolate/exp_archive.go (right): > > https://codereview.chromium.org/2514343005/diff/40001/client/cmd/isolate/exp_... > client/cmd/isolate/exp_archive.go:39: archiveOpts *isolate.ArchiveOptions > As discussed, when reading this it takes extra effort to verify that archiveOpts just points to a field within isolateFlags. I think pulling it out at the top of main is clearer. Done.
lgtm
The CQ bit was checked by djd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479861453829920,
"parent_rev": "c34ebd4c33298254cf237ed7b8113687ea583a2b", "commit_rev":
"74934dfa148729458ce3adf87904733c6e46cfe5"}
Message was sent while issue was closed.
Description was changed from ========== client/isolate: add skeleton exparchive command This experimental archive command will support archiving smaller files into large batches. It is being developed separately from the existing archive/batcharchive command to allow more freedom to explore the space and to investigate simpler pipelining of steps. Issue luci/luci-go#9 BUG=59899 R=mcgreevy@chromium.org, tansell@chromium.org ========== to ========== client/isolate: add skeleton exparchive command This experimental archive command will support archiving smaller files into large batches. It is being developed separately from the existing archive/batcharchive command to allow more freedom to explore the space and to investigate simpler pipelining of steps. Issue luci/luci-go#9 BUG=59899 R=mcgreevy@chromium.org, tansell@chromium.org Committed: https://github.com/luci/luci-go/commit/74934dfa148729458ce3adf87904733c6e46cfe5 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://github.com/luci/luci-go/commit/74934dfa148729458ce3adf87904733c6e46cfe5 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
