Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988103002:
[CT] Add ability to download to/upload from swarming GS dir with numerical subdirs (Closed)
|
Created:
4 years, 7 months ago by rmistry Modified:
4 years, 7 months ago Reviewers:
dogben CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/buildbot@ct-1-chromium_builds Target Ref:
refs/heads/master Project:
skiabuildbot Visibility:
Public. |
Description[CT] Add ability to download to/upload from swarming GS dir with numerical subdirs.
Also, increase the visibility of the deleteRemoteDir method.
BUG=skia:5245
Committed: https://skia.googlesource.com/buildbot/+/4918d794b1eeb6f308225849cf48d49071ec5734
Patch Set 1 : Initial upload #
Total comments: 12
Patch Set 2 : Address comments #Patch Set 3 : Fix vet #
Total comments: 2
Patch Set 4 : Comment #
Depends on Patchset: Messages
Total messages: 27 (12 generated)
Description was changed from ========== [CT] Add ability to download to/upload from swarming GS dir with numerical subdirs BUG=skia:5245 ========== to ========== [CT] Add ability to download to/upload from swarming GS dir with numerical subdirs. Also, increase the visibility of the deleteRemoteDir method. BUG=skia:5245 ==========
rmistry@google.com changed reviewers: + benjaminwagner@google.com
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988103002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988103002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm A couple comments below. It seems like it might be simpler to preserve the numerical subdir structure rather than building a map from artifact to index. (I'm guessing that we wouldn't have enough pagesets per slave to make the map large enough to be a concern.) https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go File ct/go/util/gs.go (right): https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode334 ct/go/util/gs.go:334: // downloads the contents of those directories into a local directory without the numerical > "without the numerical subdirs" Aren't there files with the same name in multiple subdirs? https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode377 ct/go/util/gs.go:377: } Missing return? https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode383 ct/go/util/gs.go:383: defer util.Close(respBody) If there are many items in chRemoteDirs, deferring the util.Close will prevent GCing all the respBody's, which could exhaust memory. I think it would be good to split the goroutine into a different function, since this function is quite long. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode390 ct/go/util/gs.go:390: defer util.Close(out) nit: util.Close will ignore errors on closing the file. Probably doesn't matter, since it would be errors like the disk is removed before finishing writing. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode413 ct/go/util/gs.go:413: return artifactToIndex, nil nit: return error if chRemoteDirs is not empty? https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go File ct/go/util/gs_test.go (right): https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go#newco... ct/go/util/gs_test.go:147: localDir := filepath.Join(os.TempDir(), "util_test") nit: Always prefer ioutil.TempDir over os.TempDir() to allow running multiple processes concurrently. E.g. ioutil.TempDir(nil, nil) (I guess you might use os.TempDir() for a lock file or something that should persist across runs but not across reboots, but it's hard to think of many legitimate uses.)
On 2016/05/18 at 15:22:02, Ben Wagner wrote: > It seems like it might be simpler to preserve the numerical subdir structure rather than building a map from artifact to index. filepath.Walk might be useful.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go File ct/go/util/gs.go (right): https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode334 ct/go/util/gs.go:334: // downloads the contents of those directories into a local directory without the numerical On 2016/05/18 15:22:01, Ben Wagner wrote: > > "without the numerical subdirs" > > Aren't there files with the same name in multiple subdirs? Possible but unlikely. If they exist right now they overwrite each other anyway in the current architecture. They will still overwrite each other in the new architecture but it will be even less likely due to each bot dealing with fewer artifacts than before. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode377 ct/go/util/gs.go:377: } On 2016/05/18 15:22:01, Ben Wagner wrote: > Missing return? Oops. Done. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode383 ct/go/util/gs.go:383: defer util.Close(respBody) On 2016/05/18 15:22:01, Ben Wagner wrote: > If there are many items in chRemoteDirs, deferring the util.Close will prevent > GCing all the respBody's, which could exhaust memory. > > I think it would be good to split the goroutine into a different function, since > this function is quite long. Done. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode390 ct/go/util/gs.go:390: defer util.Close(out) On 2016/05/18 15:22:01, Ben Wagner wrote: > nit: util.Close will ignore errors on closing the file. Probably doesn't matter, > since it would be errors like the disk is removed before finishing writing. Yea, I figured just logging an error was enough here. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode413 ct/go/util/gs.go:413: return artifactToIndex, nil On 2016/05/18 15:22:01, Ben Wagner wrote: > nit: return error if chRemoteDirs is not empty? Done. https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go File ct/go/util/gs_test.go (right): https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go#newco... ct/go/util/gs_test.go:147: localDir := filepath.Join(os.TempDir(), "util_test") On 2016/05/18 15:22:02, Ben Wagner wrote: > nit: Always prefer ioutil.TempDir over os.TempDir() to allow running multiple > processes concurrently. E.g. ioutil.TempDir(nil, nil) > > (I guess you might use os.TempDir() for a lock file or something that should > persist across runs but not across reboots, but it's hard to think of many > legitimate uses.) Done.
On 2016/05/18 15:22:02, Ben Wagner wrote: > lgtm > > A couple comments below. > > It seems like it might be simpler to preserve the numerical subdir structure > rather than building a map from artifact to index. I went both ways when implementing this. Having numerical subdirs in GoogleStorage and a local flat directory structure worked for all usecases. The nice thing about returning map from artifact to index is that many tasks do not have to care about that map, but they would be forced to care if the numerical subdir structure was preserved. > > (I'm guessing that we wouldn't have enough pagesets per slave to make the map > large enough to be a concern.) Yes this should not be a problem. > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go > File ct/go/util/gs.go (right): > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode334 > ct/go/util/gs.go:334: // downloads the contents of those directories into a > local directory without the numerical > > "without the numerical subdirs" > > Aren't there files with the same name in multiple subdirs? > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode377 > ct/go/util/gs.go:377: } > Missing return? > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode383 > ct/go/util/gs.go:383: defer util.Close(respBody) > If there are many items in chRemoteDirs, deferring the util.Close will prevent > GCing all the respBody's, which could exhaust memory. > > I think it would be good to split the goroutine into a different function, since > this function is quite long. > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode390 > ct/go/util/gs.go:390: defer util.Close(out) > nit: util.Close will ignore errors on closing the file. Probably doesn't matter, > since it would be errors like the disk is removed before finishing writing. > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs.go#newcode413 > ct/go/util/gs.go:413: return artifactToIndex, nil > nit: return error if chRemoteDirs is not empty? > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go > File ct/go/util/gs_test.go (right): > > https://codereview.chromium.org/1988103002/diff/1/ct/go/util/gs_test.go#newco... > ct/go/util/gs_test.go:147: localDir := filepath.Join(os.TempDir(), "util_test") > nit: Always prefer ioutil.TempDir over os.TempDir() to allow running multiple > processes concurrently. E.g. ioutil.TempDir(nil, nil) > > (I guess you might use os.TempDir() for a lock file or something that should > persist across runs but not across reboots, but it's hard to think of many > legitimate uses.)
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988103002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Infra-PerCommit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/Infra-PerCommit-Trybot/b...)
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988103002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988103002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Comment is ignorable. https://codereview.chromium.org/1988103002/diff/60001/ct/go/util/gs.go File ct/go/util/gs.go (right): https://codereview.chromium.org/1988103002/diff/60001/ct/go/util/gs.go#newcod... ct/go/util/gs.go:372: return artifactToIndex, fmt.Errorf("The chRemoteDirs channel was expected to be empty!") uber-nit: Maybe just "Unable to download all artifacts."
https://codereview.chromium.org/1988103002/diff/60001/ct/go/util/gs.go File ct/go/util/gs.go (right): https://codereview.chromium.org/1988103002/diff/60001/ct/go/util/gs.go#newcod... ct/go/util/gs.go:372: return artifactToIndex, fmt.Errorf("The chRemoteDirs channel was expected to be empty!") On 2016/05/19 14:18:20, Ben Wagner wrote: > uber-nit: Maybe just "Unable to download all artifacts." Done.
The CQ bit was checked by rmistry@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com Link to the patchset: https://codereview.chromium.org/1988103002/#ps80001 (title: "Comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988103002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988103002/80001
Message was sent while issue was closed.
Description was changed from ========== [CT] Add ability to download to/upload from swarming GS dir with numerical subdirs. Also, increase the visibility of the deleteRemoteDir method. BUG=skia:5245 ========== to ========== [CT] Add ability to download to/upload from swarming GS dir with numerical subdirs. Also, increase the visibility of the deleteRemoteDir method. BUG=skia:5245 Committed: https://skia.googlesource.com/buildbot/+/4918d794b1eeb6f308225849cf48d49071ec... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://skia.googlesource.com/buildbot/+/4918d794b1eeb6f308225849cf48d49071ec... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
