Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184293011:
Make bisect script to post build request to try server. (Closed)
|
Created:
6 years, 9 months ago by prasadv Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake bisect script to post build request job to try server.
During the course of bisect, if the script don't find build archive for a given revision on cloud storage, a request is made to try server to produce one and waits till build archive is created on storage.
Note: In this CL, only chromium revision are taken care. I'll create a separate CL for third-party repositories such as v8, webkit.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256862
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 22
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1371: Style: Both DownloadCurrentBuild and PostBuildRequestAndWait are methods of the same class, BisectPerformanceMetrics, so they should be separated by one blank line. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1379: return ('Win x64 Bisect Builder', 3600) # 60 mins Style nit: The style guide suggests spacing inline comments two spaces after the code, or putting it on a separate line. See: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1401: #Posts job to build the revision on the server. Style: Should add space after #. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1403: poll_interval = 60 Style: Unnecessary spaces. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... File tools/post_perf_builder_job.py (right): https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:6: to server by directly connecting via HTTP. Any try server? I have a general question about our infrastructure: Is it true that a "try server" is a Buildbot builder (slave) which is always listening for requests over HTTP? Anyway, it would also be good to add in the file-level comment that this file can be run independently and it can also be imported and used just for the PostTryJob function. Maybe explain in what kinds of circumstances someone might want to run this module independently, and give a usage example. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:22: def ParseArgs(options): This function could be marked as private (_ParseArgs). Also, the name of this function might be slightly confusing since there's an options.parse_args() in Main which does something totally different. Looks like the main purpose of this function is to convert the options object output by parse_args into a dictionary of query parameters which will be passed to PostTryJob. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:23: """Parse common options passed to PostTryJob.""" Style consistency note: In most places (including in the Google Python Style Guide), docstrings start with a verb in third-person singular form (i.e. "Parses" instead of "Parse", "Sends" instead of "Send"). https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:43: """Send a build request to the server using the HTTP protocol.""" This docstring could be more detailed: - Which parameters can be included in the dictionary that's passed in? Will they all be sent in the query string in a GET request? - What does returning True mean? Will False ever be returned? Or will ServerAccessError be raised in every possible situation where True isn't returned? Also, I think the PostTryJob function should be put at the top of the file, and the functions that are currently called "ParseArgs" and "gen_parser" should be put next to each other. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:78: def gen_parser(): Style: Should be GenParser (or _GenParser). Maybe it could have a better name, like "MakeOptionParser" or something similar. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:79: # Parse argv Should have a docstring with a sentence saying what the function does. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:113: Style: Should have two blank lines here.
https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1371: On 2014/03/06 23:40:45, qyearsley wrote: > Style: Both DownloadCurrentBuild and PostBuildRequestAndWait are methods of the > same class, BisectPerformanceMetrics, so they should be separated by one blank > line. Done. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1379: return ('Win x64 Bisect Builder', 3600) # 60 mins On 2014/03/06 23:40:45, qyearsley wrote: > Style nit: The style guide suggests spacing inline comments two spaces after the > code, or putting it on a separate line. See: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments. Done. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1401: #Posts job to build the revision on the server. On 2014/03/06 23:40:45, qyearsley wrote: > Style: Should add space after #. Done. https://codereview.chromium.org/184293011/diff/40001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1403: poll_interval = 60 On 2014/03/06 23:40:45, qyearsley wrote: > Style: Unnecessary spaces. Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... File tools/post_perf_builder_job.py (right): https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:6: to server by directly connecting via HTTP. The main purpose of this module is to post a request to tryserver to produce a build for given a revision. Since the tryserver already has the capability to hand such requests using TryJobHttp, we are going to use that feature for producing perf build. Currently tryserver doesn't have perf builder bots to handle this requests, infra team is going to add builder bot that are similar to Chromium.perf builders. On 2014/03/06 23:40:45, qyearsley wrote: > Any try server? > > I have a general question about our infrastructure: Is it true that a "try > server" is a Buildbot builder (slave) which is always listening for requests > over HTTP? > > Anyway, it would also be good to add in the file-level comment that this file > can be run independently and it can also be imported and used just for the > PostTryJob function. Maybe explain in what kinds of circumstances someone might > want to run this module independently, and give a usage example. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:22: def ParseArgs(options): On 2014/03/06 23:40:45, qyearsley wrote: > This function could be marked as private (_ParseArgs). > > Also, the name of this function might be slightly confusing since there's an > options.parse_args() in Main which does something totally different. > > Looks like the main purpose of this function is to convert the options object > output by parse_args into a dictionary of query parameters which will be passed > to PostTryJob. Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:23: """Parse common options passed to PostTryJob.""" On 2014/03/06 23:40:45, qyearsley wrote: > Style consistency note: In most places (including in the Google Python Style > Guide), docstrings start with a verb in third-person singular form (i.e. > "Parses" instead of "Parse", "Sends" instead of "Send"). Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:43: """Send a build request to the server using the HTTP protocol.""" On 2014/03/06 23:40:45, qyearsley wrote: > This docstring could be more detailed: > > - Which parameters can be included in the dictionary that's passed in? Will > they all be sent in the query string in a GET request? > - What does returning True mean? Will False ever be returned? Or will > ServerAccessError be raised in every possible situation where True isn't > returned? > > Also, I think the PostTryJob function should be put at the top of the file, and > the functions that are currently called "ParseArgs" and "gen_parser" should be > put next to each other. Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:78: def gen_parser(): On 2014/03/06 23:40:45, qyearsley wrote: > Style: Should be GenParser (or _GenParser). Maybe it could have a better name, > like "MakeOptionParser" or something similar. Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:79: # Parse argv On 2014/03/06 23:40:45, qyearsley wrote: > Should have a docstring with a sentence saying what the function does. Done. https://codereview.chromium.org/184293011/diff/40001/tools/post_perf_builder_... tools/post_perf_builder_job.py:113: On 2014/03/06 23:40:45, qyearsley wrote: > Style: Should have two blank lines here. Done.
lg2m, but simon should approve https://codereview.chromium.org/184293011/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:3194: group.add_option('--host', Recommend names like --builder-host and --builder-port. https://codereview.chromium.org/184293011/diff/60001/tools/post_perf_builder_... File tools/post_perf_builder_job.py (right): https://codereview.chromium.org/184293011/diff/60001/tools/post_perf_builder_... tools/post_perf_builder_job.py:37: if not url_params.get('host'): These missing param checks should probably raise a ValueError instead of ServerAccessError
https://codereview.chromium.org/184293011/diff/60001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/60001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:3194: group.add_option('--host', On 2014/03/07 02:41:09, tonyg wrote: > Recommend names like --builder-host and --builder-port. Done. https://codereview.chromium.org/184293011/diff/60001/tools/post_perf_builder_... File tools/post_perf_builder_job.py (right): https://codereview.chromium.org/184293011/diff/60001/tools/post_perf_builder_... tools/post_perf_builder_job.py:37: if not url_params.get('host'): On 2014/03/07 02:41:09, tonyg wrote: > These missing param checks should probably raise a ValueError instead of > ServerAccessError Done.
Sorry for letting this slide! Looks fine to me overall, lgtm https://codereview.chromium.org/184293011/diff/80001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/80001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1409: if elapsed_time > build_timeout: A timeout seems fair enough for now I think. I can imagine once sheriff's are using this, if several people post build jobs (to say, windows), that we might be mid-build when this timeout occurs. Is the plan to add additional build capacity to handle this? Is there any way to check what the status of the build is, in case it's *nearly* done?
https://codereview.chromium.org/184293011/diff/80001/tools/bisect-perf-regres... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/184293011/diff/80001/tools/bisect-perf-regres... tools/bisect-perf-regression.py:1409: if elapsed_time > build_timeout: To handle load, we requested 16 new builder bots on tryserver (crbug.com/347707). Regarding build status, I'll investigate a way to find this information. On 2014/03/11 17:20:54, shatch wrote: > A timeout seems fair enough for now I think. I can imagine once sheriff's are > using this, if several people post build jobs (to say, windows), that we might > be mid-build when this timeout occurs. Is the plan to add additional build > capacity to handle this? Is there any way to check what the status of the build > is, in case it's *nearly* done?
The CQ bit was checked by prasadv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prasadv@chromium.org/184293011/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by prasadv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prasadv@chromium.org/184293011/80001
Message was sent while issue was closed.
Change committed as 256862 |
