Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130543004:
Waterfall components of regression range finder. (Closed)
DescriptionWaterfall components of regression range finder. 2/3
BUG=617808
Committed: https://chromium.googlesource.com/infra/infra/+/f1e17901719e3b7fe844e2a06f7fe4add6c17155
Patch Set 1 #
Total comments: 50
Patch Set 2 : refactored #
Total comments: 15
Patch Set 3 : Cleanup #Patch Set 4 : cleanup #
Total comments: 30
Patch Set 5 : addressed comments #Patch Set 6 : linted, tests #
Total comments: 10
Patch Set 7 : addressed comments, added trigger test which somehow wasn't included last time #
Total comments: 18
Patch Set 8 : addressed comments #Patch Set 9 : I now test everything that I wrote. #
Total comments: 37
Patch Set 10 : addressed comments #
Total comments: 10
Patch Set 11 : addressed comments #
Total comments: 5
Patch Set 12 : addressed comments #Patch Set 13 : addressed comments #Patch Set 14 : addressed comments #Patch Set 15 : Merge branch 'master' of https://chromium.googlesource.com/infra/infra into flake-waterfall #Patch Set 16 : added init #Patch Set 17 : added another init whoooo #Messages
Total messages: 46 (16 generated)
caiw@google.com changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org, stgao@chromium.org
I cut up the provisionally running pipeline into 3 CLs. This one contains the actual pipelines and their initializers which are run on the waterfall backend.
Description was changed from ========== Waterfall components of regression range finder. BUG=617808 ========== to ========== Waterfall components of regression range finder. 2/3 BUG=617808 ==========
I didn't go through all the code. I just have a question: if I understand right, the 2 base pipelines are common code for current swarming pipelines and new swarming regression pipelines? If so I think maybe we can also use helper module instead of base pipelines? Or if you and Shuotao have decided to do it in this way, I'll continue reviewing the code as is. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: from model import analysis_status Move ln 16 before ln 10 https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:19: Only 2 empty lines are needed here. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:23: master_name, builder_name, step_name, build_number): builder_name is not used in function? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:26: A MasterFlakeAnalysis entity for the given build will be created if none I thought MasterFlakeAnalysis is by (master_name, builder_name, step_name)? Although as I mentioned in CL 1, I think we need this at test level rather than step level. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:40: # as long as it is not completed It is fine for now that you have this kind of comments in code since the code is still under development, but make sure remove them when you're ready to commit you code. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:48: elif (analysis.status != analysis_status.COMPLETED): what if analysis.status == PENDING or RUNNING? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:51: MasterFlakeAnalysis.Get(master_name, builder_name, step_name).key.delete() Alternative: write a Reset function instead. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:59: if analysis.status == analysis_status.COMPLETED: This is actually else after elif above, right? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:47: class NextBuildNumberPipeline(BasePipeline): Any reason you wrote these 2 classes in the same file?
https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: from model import analysis_status imports of the same category are usually ordered alphabetically https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:35: # I think this flow has some redundancy with check_flake handler The logic to determine whether an analysis is needed or not should be here within a NDB transaction. The check_flake handler could just call function ``ScheduleAnalysisIfNeeded`` below regardless to which status the analysis is, and then render the result based on the MasterFlakeAnalysis returned from the function. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:13: from model.flake.master_flake_analysis import MasterFlakeAnalysis as MFA I understand the class name is a bit long, but the convention in the code base is to avoid abbreviations because they could cause confusion. Same for lines #15 and #17. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:26: self.master_name = master_name Why do we need to save these values? they seems not used. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:42: step_future = yield PFSTRP( rename ``step_future`` to "test_result_future``? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:58: # Placeholder until we develop an algorithm Add a TODO here? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:29: output_json (dict): A dict of all test results in the swarming task. style nit: indent. Args: arg_name (type): explanation. You may also run pylint to check for most style violations: pylint path/to/python_file.py https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:64: pass How about raising an NotImplementedError here as the subclass is expected to implement it? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:36: output_json (dict): A dict of all test results in the swarming task. style nit: indent. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:61: mfa.success_rates.append(successes * 1.0/tries) This might have to be out of both for loops, because each test will show up in all iterations. You may want to check one of a Swarming run result (i.e. https://chromium-swarm.appspot.com/user/task/2fdbf62cc33ed210) for the data format. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:77: return swarming_task nit: merge into one line. Does the read have to be in a transaction? What's the consideration behind that? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:75: pass Raise NotImplementedError ? Same for below. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:85: @ndb.transactional(xg=True) Why cross-group transaction is needed here? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:124: iterations_to_rerun = waterfall_config.GetSwarmingSettings().get( Should this be implemented in the subclass instead? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:14: @ndb.transactional Why this is needed? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:21: @ndb.transactional Why this is needed too? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:32: return 10 How about adding a TODO here to move this into the config?
https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: from model import analysis_status On 2016/07/08 16:58:54, chanli wrote: > Move ln 16 before ln 10 Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:16: from model import analysis_status On 2016/07/09 00:04:33, stgao wrote: > imports of the same category are usually ordered alphabetically Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:19: On 2016/07/08 16:58:54, chanli wrote: > Only 2 empty lines are needed here. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:23: master_name, builder_name, step_name, build_number): On 2016/07/08 16:58:53, chanli wrote: > builder_name is not used in function? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:26: A MasterFlakeAnalysis entity for the given build will be created if none On 2016/07/08 16:58:54, chanli wrote: > I thought MasterFlakeAnalysis is by (master_name, builder_name, step_name)? > Although as I mentioned in CL 1, I think we need this at test level rather than > step level. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:35: # I think this flow has some redundancy with check_flake handler On 2016/07/09 00:04:33, stgao wrote: > The logic to determine whether an analysis is needed or not should be here > within a NDB transaction. > > The check_flake handler could just call function ``ScheduleAnalysisIfNeeded`` > below regardless to which status the analysis is, and then render the result > based on the MasterFlakeAnalysis returned from the function. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:40: # as long as it is not completed On 2016/07/08 16:58:54, chanli wrote: > It is fine for now that you have this kind of comments in code since the code is > still under development, but make sure remove them when you're ready to commit > you code. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:48: elif (analysis.status != analysis_status.COMPLETED): On 2016/07/08 16:58:54, chanli wrote: > what if analysis.status == PENDING or RUNNING? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:51: MasterFlakeAnalysis.Get(master_name, builder_name, step_name).key.delete() On 2016/07/08 16:58:54, chanli wrote: > Alternative: write a Reset function instead. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:59: if analysis.status == analysis_status.COMPLETED: On 2016/07/08 16:58:54, chanli wrote: > This is actually else after elif above, right? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:13: from model.flake.master_flake_analysis import MasterFlakeAnalysis as MFA On 2016/07/09 00:04:33, stgao wrote: > I understand the class name is a bit long, but the convention in the code base > is to avoid abbreviations because they could cause confusion. > > Same for lines #15 and #17. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:42: step_future = yield PFSTRP( On 2016/07/09 00:04:33, stgao wrote: > rename ``step_future`` to "test_result_future``? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:47: class NextBuildNumberPipeline(BasePipeline): On 2016/07/08 16:58:54, chanli wrote: > Any reason you wrote these 2 classes in the same file? Is there a convention of one class per file? I thought this made sense because nextbuildnumberpipeline is sort of a helper function of recursiveflakepipeline, except it has to be a pipeline because of the appengine. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:58: # Placeholder until we develop an algorithm On 2016/07/09 00:04:33, stgao wrote: > Add a TODO here? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:29: output_json (dict): A dict of all test results in the swarming task. On 2016/07/09 00:04:33, stgao wrote: > style nit: indent. > > Args: > arg_name (type): explanation. > > You may also run pylint to check for most style violations: > pylint path/to/python_file.py Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:64: pass On 2016/07/09 00:04:33, stgao wrote: > How about raising an NotImplementedError here as the subclass is expected to > implement it? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:36: output_json (dict): A dict of all test results in the swarming task. On 2016/07/09 00:04:34, stgao wrote: > style nit: indent. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:61: mfa.success_rates.append(successes * 1.0/tries) On 2016/07/09 00:04:34, stgao wrote: > This might have to be out of both for loops, because each test will show up in > all iterations. > > You may want to check one of a Swarming run result (i.e. > https://chromium-swarm.appspot.com/user/task/2fdbf62cc33ed210) for the data > format. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:75: pass On 2016/07/09 00:04:34, stgao wrote: > Raise NotImplementedError ? > > Same for below. Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:85: @ndb.transactional(xg=True) On 2016/07/09 00:04:34, stgao wrote: > Why cross-group transaction is needed here? I don't remember, but I got an error requiring me to set this to true. it might be because of the relation between swarmingtask and master_flake_analysis? https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:124: iterations_to_rerun = waterfall_config.GetSwarmingSettings().get( On 2016/07/09 00:04:34, stgao wrote: > Should this be implemented in the subclass instead? Sure, that would be fine. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:14: @ndb.transactional On 2016/07/09 00:04:34, stgao wrote: > Why this is needed? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:21: @ndb.transactional On 2016/07/09 00:04:34, stgao wrote: > Why this is needed too? Done. https://codereview.chromium.org/2130543004/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:32: return 10 On 2016/07/09 00:04:34, stgao wrote: > How about adding a TODO here to move this into the config? Done.
https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:42: analysis.status == analysis_status.PENDING): What if it is running? https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:27: self.master_name = master_name Still, I'm wondering why we need to save these unused values? https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:44: test_result_future = yield PFSTRP( style nit: avoid abbreviation. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:69: HTTP_CLIENT = HttpClient() style nit: move this up before all class methods. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:134: logging.info(call_args) I assume these loggings are for debugging and testing. They should be removed before commit. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:70: @ndb.transactional Why this one also need transaction? Same for _GetSwarmingTask below. Maybe that's why there was an error regarding the cross-group transaction below. Would you mind a verification? https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:92: swarming_task = self._GetSwarmingTask(*args) As we only read and write the same SwarmingTask entity, it should not be cross-group transaction. See my comment above too. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:36: def _GetArgs(self, master_name, builder_name, build_number, step_name, tests): One alternative is to return the key of the entity.
Addressed previous comments https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:42: analysis.status == analysis_status.PENDING): On 2016/07/14 18:34:50, stgao wrote: > What if it is running? Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:27: self.master_name = master_name On 2016/07/14 18:34:50, stgao wrote: > Still, I'm wondering why we need to save these unused values? Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:44: test_result_future = yield PFSTRP( On 2016/07/14 18:34:50, stgao wrote: > style nit: avoid abbreviation. Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:69: HTTP_CLIENT = HttpClient() On 2016/07/14 18:34:50, stgao wrote: > style nit: move this up before all class methods. Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:70: @ndb.transactional On 2016/07/14 18:34:50, stgao wrote: > Why this one also need transaction? Same for _GetSwarmingTask below. > > Maybe that's why there was an error regarding the cross-group transaction below. > Would you mind a verification? Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:92: swarming_task = self._GetSwarmingTask(*args) On 2016/07/14 18:34:50, stgao wrote: > As we only read and write the same SwarmingTask entity, it should not be > cross-group transaction. See my comment above too. Done. https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:36: def _GetArgs(self, master_name, builder_name, build_number, step_name, tests): On 2016/07/14 18:34:50, stgao wrote: > One alternative is to return the key of the entity. I thought we talked and said this was ok for now?
https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2016 https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:6: import logging Remove this import https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:44: else: First: Maybe you can create the Reset method and remove the TODO? Second: Here you just delete the entity right? But in docstring you said the result of last analysis will be kept? https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, Is force saved for later change? https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. 2015 -> 2016 https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:6: import logging Remove this import? https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:41: # Get MasterFlakeAnalysis success list corresponding to parameters Nit: Gets ... and add '.' at the end. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:27: def _CheckTestsRunStatuses(self, output_json): It seems this method will be overridden? 'raise NotImplementedError' should be enough right?
Just some initial comments for now, I'll review the next patchset in more detail once comments are addressed https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:6: import logging On 2016/07/19 20:48:27, chanli wrote: > Remove this import If you run gpylint <path to python file> it should point out unused imports like this https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:25: # Call trigger pipeline (flake style) nit: comments end with . https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:47: if len(master.build_numbers) < 10: It looks like 10 is used twice and not sure if it's related to line 49. If it is, move it to a constant at the top of the file. If 10 is subject to change I'd make it configurable and add a todo here that this should be set in Findit's config (see model/wf_config.py and waterfall/waterfall_config.py if you're curious) https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:48: #TODO(caiw): Develop algorithm to optimize this. nit: space after # https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:42: Returns: nit: empty line before Returns https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:49: mfa = MasterFlakeAnalysis.Get(master_name, builder_name, nit: it's ok to use longer variable names with the spelled out words master_flake_analysis = ... flake_swarming_task = ... etc. for consistency with the rest of the code base
https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/19 20:48:27, chanli wrote: > 2014 -> 2016 Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:6: import logging On 2016/07/19 20:48:27, chanli wrote: > Remove this import Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:44: else: On 2016/07/19 20:48:28, chanli wrote: > First: Maybe you can create the Reset method and remove the TODO? > > Second: Here you just delete the entity right? But in docstring you said the > result of last analysis will be kept? So this is only if the status of the last analysis is an error. If that's the case, I wanted to delete it and just run it again. The todo is just a reminder to move this logic into a reset method - I was hoping to do that after we got the CL committed. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, On 2016/07/19 20:48:28, chanli wrote: > Is force saved for later change? Yes - I think later we will need to figure out who has the permissions to run these things? I am not completely clear on how it works. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/19 20:48:28, chanli wrote: > 2015 -> 2016 Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:6: import logging On 2016/07/19 20:48:28, chanli wrote: > Remove this import? Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:25: # Call trigger pipeline (flake style) On 2016/07/19 22:44:10, lijeffrey wrote: > nit: comments end with . Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:47: if len(master.build_numbers) < 10: On 2016/07/19 22:44:10, lijeffrey wrote: > It looks like 10 is used twice and not sure if it's related to line 49. If it > is, move it to a constant at the top of the file. > > If 10 is subject to change I'd make it configurable and add a todo here that > this should be set in Findit's config (see model/wf_config.py and > waterfall/waterfall_config.py if you're curious) So they aren't related, and also the algorithm (which is currently in development in a googledoc somewhere if you want to take a look!) doesn't use this same logic of checking if we did x number of swarming reruns and then stop, so I think it's not worth it to set it in the config since the next CL I submit will probably remove this anyways. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:48: #TODO(caiw): Develop algorithm to optimize this. On 2016/07/19 22:44:10, lijeffrey wrote: > nit: space after # Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:42: Returns: On 2016/07/19 22:44:10, lijeffrey wrote: > nit: empty line before Returns Done. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:49: mfa = MasterFlakeAnalysis.Get(master_name, builder_name, On 2016/07/19 22:44:10, lijeffrey wrote: > nit: it's ok to use longer variable names with the spelled out words > > master_flake_analysis = ... > flake_swarming_task = ... > etc. > > for consistency with the rest of the code base Done.
https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:44: else: On 2016/07/20 18:26:47, caiw wrote: > On 2016/07/19 20:48:28, chanli wrote: > > First: Maybe you can create the Reset method and remove the TODO? > > > > Second: Here you just delete the entity right? But in docstring you said the > > result of last analysis will be kept? > > So this is only if the status of the last analysis is an error. If that's the > case, I wanted to delete it and just run it again. The todo is just a reminder > to move this logic into a reset method - I was hoping to do that after we got > the CL committed. Acknowledged. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, On 2016/07/20 18:26:47, caiw wrote: > On 2016/07/19 20:48:28, chanli wrote: > > Is force saved for later change? > > Yes - I think later we will need to figure out who has the permissions to run > these things? I am not completely clear on how it works. I think we can add force when we want to do it?
+ tests + lint https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, On 2016/07/21 20:34:09, chanli wrote: > On 2016/07/20 18:26:47, caiw wrote: > > On 2016/07/19 20:48:28, chanli wrote: > > > Is force saved for later change? > > > > Yes - I think later we will need to figure out who has the permissions to run > > these things? I am not completely clear on how it works. > > I think we can add force when we want to do it? So should we do it now?
https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, On 2016/07/22 21:46:42, caiw wrote: > On 2016/07/21 20:34:09, chanli wrote: > > On 2016/07/20 18:26:47, caiw wrote: > > > On 2016/07/19 20:48:28, chanli wrote: > > > > Is force saved for later change? > > > > > > Yes - I think later we will need to figure out who has the permissions to > run > > > these things? I am not completely clear on how it works. > > > > I think we can add force when we want to do it? > > So should we do it now? I would say add it in a separated CL, even the change is small. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... File appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. Nit: 2015 -> 2016 https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:6: import mock This import should be the first one and in a separated group https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:61: analysis_status.COMPLETED]: Maybe use different build_number or test_name to create different MasterFlakeAnalysis? https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... File appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:86: def my_mocked_run(arg1, queue_name): I'm not sure if that'll work, but maybe you can try: def my_mocked_run(*_): queue_name['x'] = True
addressed comments. also, somehow the test for triggering wasn't included last time and seemed to have been lost somewhere in git, so I rewrote it and included it. https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:57: test_name, force=False, On 2016/07/26 17:41:05, chanli wrote: > On 2016/07/22 21:46:42, caiw wrote: > > On 2016/07/21 20:34:09, chanli wrote: > > > On 2016/07/20 18:26:47, caiw wrote: > > > > On 2016/07/19 20:48:28, chanli wrote: > > > > > Is force saved for later change? > > > > > > > > Yes - I think later we will need to figure out who has the permissions to > > run > > > > these things? I am not completely clear on how it works. > > > > > > I think we can add force when we want to do it? > > > > So should we do it now? > > I would say add it in a separated CL, even the change is small. Acknowledged. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... File appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/26 17:41:05, chanli wrote: > Nit: 2015 -> 2016 Done. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:6: import mock On 2016/07/26 17:41:05, chanli wrote: > This import should be the first one and in a separated group Got it - does that mean I shouldn't trust gpylint for imports? Because I copy pasted this ordering from gpylint. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/initialize_flake_pipeline_test.py:61: analysis_status.COMPLETED]: On 2016/07/26 17:41:05, chanli wrote: > Maybe use different build_number or test_name to create different > MasterFlakeAnalysis? Done. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... File appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/07/26 17:41:05, chanli wrote: > 2016 Done. https://codereview.chromium.org/2130543004/diff/100001/appengine/findit/water... appengine/findit/waterfall/flake/test/recursive_flake_pipeline_test.py:86: def my_mocked_run(arg1, queue_name): On 2016/07/26 17:41:05, chanli wrote: > I'm not sure if that'll work, but maybe you can try: > def my_mocked_run(*_): > queue_name['x'] = True Done.
https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:69: Returns: nit: empty line before Returns section https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:44: # This is a placeholder for testing: What do you mean this is a placeholder for testing? So do you plan to remove this before committing? If so in general it's better to avoid putting that in the code if possible to prevent accidentally committing it (been there done that it wasn't fun) https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:46: if len(master.build_numbers) < 10: nit: Move 10 to a constant at the top of the file https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:18: class ProcessBaseSwarmingTaskResultPipeline(BasePipeline): # pragma: no cover are you sure you want to no cover this whole class? https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:32: def _ConvertDateTime(self, time_string): is there a reason for moving this into the class? In this case it probably doesn't matter, just wondering why you've designed it this way https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:49: raise NotImplementedError you should be able to do raise NotImplementedError('some message where it should be implemented instead') https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:97: # This following line will not be compatible! why not? Should there be a TODO here to fix it? https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:60: if output_json: nit: another way to do this to prevent too many layers of nesting which can get complicated to read is to do if not output_json: return test_statuses successes = 0 tries = 0 for iteration ... ... https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:35: return 10 this should already be in config. see waterfall_config.GetSwarmingSettings()
https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:69: Returns: On 2016/07/26 20:49:02, lijeffrey wrote: > nit: empty line before Returns section Done. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:44: # This is a placeholder for testing: On 2016/07/26 20:49:02, lijeffrey wrote: > What do you mean this is a placeholder for testing? So do you plan to remove > this before committing? If so in general it's better to avoid putting that in > the code if possible to prevent accidentally committing it (been there done that > it wasn't fun) I think I addressed this and the comment about the number 10 in a previous patch. In the next CL there is going to be a more intelligent algorithm which will replace the current method of just going back 10 build numbers ten times so this is just scaffolding code to make the pipeline work until that algorithm is introduced. I think this was actually already OKed by someone. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/flake/recursive_flake_pipeline.py:46: if len(master.build_numbers) < 10: On 2016/07/26 20:49:02, lijeffrey wrote: > nit: Move 10 to a constant at the top of the file Acknowledged. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:18: class ProcessBaseSwarmingTaskResultPipeline(BasePipeline): # pragma: no cover On 2016/07/26 20:49:02, lijeffrey wrote: > are you sure you want to no cover this whole class? Yes - the problem is that it's basically impossible to actually to test this class on its own, so I am just testing it in the process_flake..._test.py file, but the check for test coverage doesn't count that. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:32: def _ConvertDateTime(self, time_string): On 2016/07/26 20:49:02, lijeffrey wrote: > is there a reason for moving this into the class? In this case it probably > doesn't matter, just wondering why you've designed it this way Yes - it makes it easier to extend the class, which is the point of refactoring into base. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:49: raise NotImplementedError On 2016/07/26 20:49:02, lijeffrey wrote: > you should be able to do > > raise NotImplementedError('some message where it should be implemented instead') Done. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:97: # This following line will not be compatible! On 2016/07/26 20:49:02, lijeffrey wrote: > why not? Should there be a TODO here to fix it? oops - I think that was just a comment I forgot to delete. I think it should be Ok. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:60: if output_json: On 2016/07/26 20:49:02, lijeffrey wrote: > nit: another way to do this to prevent too many layers of nesting which can get > complicated to read is to do > > if not output_json: > return test_statuses > > successes = 0 > tries = 0 > > for iteration ... > ... Done. https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... File appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/120001/appengine/findit/water... appengine/findit/waterfall/trigger_flake_swarming_task_pipeline.py:35: return 10 On 2016/07/26 20:49:02, lijeffrey wrote: > this should already be in config. see waterfall_config.GetSwarmingSettings() removed the todo. Since the next cl won't use this method, this is also basically just scaffolding.
https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:25: reset the existing one but still keep the result of last analysis. so where is this result being kept exactly? https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:30: analysis = MasterFlakeAnalysis.Get(master_name, builder_name, build_number, nit: rename this master_flake_analysis instead of just analysis. In a lot of the parts of the code we're using analysis = WfAnalysis ... so for consistency rename this variable https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:30: raise NotImplementedError nit: message for where this should be implemented https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:35: # According to swarming.py, nit: convertion -> conversion? Also no need for so many line breaks in this comment. # Match the time conversion with swarming.py which elides the suffix # when microsecionds are 0. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:49: raise NotImplementedError('Must implement _GetSwarmingTask') nit: "_GetSwarmingTask should be implemented in the child class" same with the others https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:19: ProcessBaseSwarmingTaskResultPipeline as PBSTRP) I know this is a long name but it's probably better just to spell it out for consistency with the other parts of the code and for readability https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:58: if not output_json: it looks like you may be able to bail out even sooner before. This way you save 2 datastore reads. test_statuses = defaultdict(...) if not output_json: return test_statuses master_flake_analysis = ... flake_swarming_task = ... https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:71: master_flake_analysis.build_numbers.append(build_number) nit: empty line before master_flake_analysis.build_numbers.append(...) for readability https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:89: # Get the appropriate kind of Swarming Task (Flake) nit: comment ends with . You should be able to run gpylint <path to python file> that will point these out for you https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:22: self.assertEqual(test_time,time) nit: space after , https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:30: time_string = 'fdjasklfdhjsakfhjdkalsf' lol. use 'abc' or something https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:34: self.assertEqual(1,2) # pragma: no cover what? https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/trigger_flake_swarming_task_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/trigger_flake_swarming_task_pipeline_test.py:144: print json.dumps(expected_new_request_json, sort_keys=True) make sure you take these prints out before committing the code https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:20: """A pipeline to trigger a Swarming task to re-run selected tests of a step. nit: space after #. Didn't we discuss taking out this pragma no cover and adding it at the run method instead? https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:74: raise NotImplementedError nit: remember to add where this should be implemented in the message https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:99: deadline = time.time() + 5 * 60 # Wait for 5 minutes. nit: Should this be configurable? If so move this to a constant at the top of the file # TODO(caiw): Move this to config. (if it's not already in config that is) FIVE_MINUTES = 5 * 60 https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:104: raise Exception('Swarming task was deleted unexpectedly!!!') nit: just 1 ! is fine https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:110: time.sleep(10) is this seconds? I would also move this to a constant at the top of the file with a TODO that it belongs in config. Ideally you should move it there but I don't expect you to know much about findit's config page at this stage
https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:25: reset the existing one but still keep the result of last analysis. On 2016/07/27 22:38:22, lijeffrey wrote: > so where is this result being kept exactly? sorry, docstring was outdated https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:30: analysis = MasterFlakeAnalysis.Get(master_name, builder_name, build_number, On 2016/07/27 22:38:22, lijeffrey wrote: > nit: rename this master_flake_analysis instead of just analysis. In a lot of the > parts of the code we're using analysis = WfAnalysis ... so for consistency > rename this variable Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:30: raise NotImplementedError On 2016/07/27 22:38:23, lijeffrey wrote: > nit: message for where this should be implemented Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:35: # According to swarming.py, On 2016/07/27 22:38:22, lijeffrey wrote: > nit: convertion -> conversion? > > Also no need for so many line breaks in this comment. > > # Match the time conversion with swarming.py which elides the suffix > # when microsecionds are 0. Haha I actually didn't write this but changed. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_base_swarming_task_result_pipeline.py:49: raise NotImplementedError('Must implement _GetSwarmingTask') On 2016/07/27 22:38:23, lijeffrey wrote: > nit: "_GetSwarmingTask should be implemented in the child class" same with the > others Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:19: ProcessBaseSwarmingTaskResultPipeline as PBSTRP) On 2016/07/27 22:38:23, lijeffrey wrote: > I know this is a long name but it's probably better just to spell it out for > consistency with the other parts of the code and for readability Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:58: if not output_json: On 2016/07/27 22:38:23, lijeffrey wrote: > it looks like you may be able to bail out even sooner before. This way you save > 2 datastore reads. > > test_statuses = defaultdict(...) > > if not output_json: > return test_statuses > > master_flake_analysis = ... > flake_swarming_task = ... Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:71: master_flake_analysis.build_numbers.append(build_number) On 2016/07/27 22:38:23, lijeffrey wrote: > nit: empty line before master_flake_analysis.build_numbers.append(...) for > readability Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:89: # Get the appropriate kind of Swarming Task (Flake) On 2016/07/27 22:38:23, lijeffrey wrote: > nit: comment ends with . > > You should be able to run gpylint <path to python file> that will point these > out for you Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:22: self.assertEqual(test_time,time) On 2016/07/27 22:38:23, lijeffrey wrote: > nit: space after , Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:30: time_string = 'fdjasklfdhjsakfhjdkalsf' On 2016/07/27 22:38:23, lijeffrey wrote: > lol. use 'abc' or something Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:34: self.assertEqual(1,2) # pragma: no cover On 2016/07/27 22:38:23, lijeffrey wrote: > what? The idea is that if the code is working properly it won't get here. I don't know if this is the appropriate way to do this. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/trigger_flake_swarming_task_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/trigger_flake_swarming_task_pipeline_test.py:144: print json.dumps(expected_new_request_json, sort_keys=True) On 2016/07/27 22:38:23, lijeffrey wrote: > make sure you take these prints out before committing the code Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:20: """A pipeline to trigger a Swarming task to re-run selected tests of a step. On 2016/07/27 22:38:23, lijeffrey wrote: > nit: space after #. Didn't we discuss taking out this pragma no cover and adding > it at the run method instead? So we talked about that for the process one. For this one, I thought you told me I should only write tests for what I wrote, and the only method I touched was run which we discussed should be pragma-d. I think the others should all be run by the test trigger flake pipeline. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:74: raise NotImplementedError On 2016/07/27 22:38:23, lijeffrey wrote: > nit: remember to add where this should be implemented in the message Done. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:99: deadline = time.time() + 5 * 60 # Wait for 5 minutes. On 2016/07/27 22:38:23, lijeffrey wrote: > nit: Should this be configurable? If so move this to a constant at the top of > the file > > # TODO(caiw): Move this to config. (if it's not already in config that is) > FIVE_MINUTES = 5 * 60 I have no idea if this should be configurable - in general are there guidelines for that? Shuotao wrote this piece of code I think. https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:104: raise Exception('Swarming task was deleted unexpectedly!!!') On 2016/07/27 22:38:23, lijeffrey wrote: > nit: just 1 ! is fine Done, (again I didn't write this) https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:110: time.sleep(10) On 2016/07/27 22:38:23, lijeffrey wrote: > is this seconds? I would also move this to a constant at the top of the file > with a TODO that it belongs in config. Ideally you should move it there but I > don't expect you to know much about findit's config page at this stage I have no idea if this is seconds: again I didn't write it haha.
lgtm with a nit https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:25: analysis Should this analysis be removed?
https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/160001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:20: """A pipeline to trigger a Swarming task to re-run selected tests of a step. On 2016/07/27 23:51:33, caiw wrote: > On 2016/07/27 22:38:23, lijeffrey wrote: > > nit: space after #. Didn't we discuss taking out this pragma no cover and > adding > > it at the run method instead? > > So we talked about that for the process one. For this one, I thought you told > me I should only write tests for what I wrote, and the only method I touched was > run which we discussed should be pragma-d. I think the others should all be run > by the test trigger flake pipeline. Cool if you can take this one out and still get 100% code coverage then do that, else it's fine to leave it as it is https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:68: master_flake_analysis.success_rates.append(successes * 1.0/tries) nit: it looks like you just want a float here so you should be able to do .append(float(successes) / tries) or whatever combination that gets you the value you want instead of the * 1.0 / tries hackery. Be careful of float(success / tries) which I believe will always floor your value https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:33: test_time = self.pipeline._ConvertDateTime(time_string) ah you're trying to get a raises. There should be a self.assertRaises(ValueError, ... some other args I forgot) so do a grep for assertRaises in the codebase for examples. This way you can get rid of the try except/pass/assertEqual(1,2) hackery https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:102: sleep_time = 10 Move these to the top of the file after the imports: import blahblah import blahblah SLEEP_TIME = 10 DEADLINE_SECONDS = 5 * 60 # 5 minutes. class TriggerBaseSwarmingTaskPipeLine(...): This way they're easy to find and configure/move to config later https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:103: deadline = time.time() + 5 * 60 # Wait for 5 minutes. then here can be time.time() + DEADLINE_SECONDS etc.
https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/flake/initialize_flake_pipeline.py:25: analysis On 2016/07/28 21:45:28, chanli wrote: > Should this analysis be removed? dunno how this got cut off but I think I accidentally deleted a line of comments https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:68: master_flake_analysis.success_rates.append(successes * 1.0/tries) On 2016/07/29 05:18:39, lijeffrey wrote: > nit: it looks like you just want a float here so you should be able to do > .append(float(successes) / tries) or whatever combination that gets you the > value you want instead of the * 1.0 / tries hackery. Be careful of float(success > / tries) which I believe will always floor your value Done. https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:33: test_time = self.pipeline._ConvertDateTime(time_string) On 2016/07/29 05:18:39, lijeffrey wrote: > ah you're trying to get a raises. There should be a > self.assertRaises(ValueError, ... some other args I forgot) so do a grep for > assertRaises in the codebase for examples. This way you can get rid of the try > except/pass/assertEqual(1,2) hackery Done. https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:102: sleep_time = 10 On 2016/07/29 05:18:39, lijeffrey wrote: > Move these to the top of the file after the imports: > > > import blahblah > import blahblah > > > SLEEP_TIME = 10 > DEADLINE_SECONDS = 5 * 60 # 5 minutes. > > > class TriggerBaseSwarmingTaskPipeLine(...): > > This way they're easy to find and configure/move to config later Done. https://codereview.chromium.org/2130543004/diff/180001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:103: deadline = time.time() + 5 * 60 # Wait for 5 minutes. On 2016/07/29 05:18:39, lijeffrey wrote: > then here can be time.time() + DEADLINE_SECONDS etc. Done.
lgtm with nits. Feel free to commit once the cleanup is done. https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... File appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... appengine/findit/waterfall/process_flake_swarming_task_result_pipeline.py:68: master_flake_analysis.success_rates.append(float(successes)/tries) nit: space before and after / https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... File appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py (right): https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... appengine/findit/waterfall/test/process_base_swarming_task_result_pipeline_test.py:32: self.pipeline._ConvertDateTime(time_string) nit: you can just pass 'abc' directly instead of storing it in time_string https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... File appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py (right): https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:20: DEADLINE_SECONDS = 5 * 60 # 5 minutes nit: comment ends with . https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:107: deadline = time.time() + DEADLINE_SECONDS # Wait for 5 minutes. nit: The comment here is no longer needed and may not necessarily be 5 minutes so just take it out. https://codereview.chromium.org/2130543004/diff/200001/appengine/findit/water... appengine/findit/waterfall/trigger_base_swarming_task_pipeline.py:118: time.sleep(SLEEP_TIME) nit: Sorry make this SLEEP_TIME_SECONDS for readability.
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2130543004/#ps240001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30511a92f405bf10) Infra Mac Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30511a91c9f65010) Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30511a9169277010)
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2130543004/#ps260001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30512ea071da3c10) Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30512ea08159a610)
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2130543004/#ps280001 (title: "Merge branch 'master' of https://chromium.googlesource.com/infra/infra into flake-waterfall")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2130543004/#ps300001 (title: "added init")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3051509a26928610)
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2130543004/#ps320001 (title: "added another init whoooo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Waterfall components of regression range finder. 2/3 BUG=617808 ========== to ========== Waterfall components of regression range finder. 2/3 BUG=617808 Committed: https://chromium.googlesource.com/infra/infra/+/f1e17901719e3b7fe844e2a06f7fe... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/infra/infra/+/f1e17901719e3b7fe844e2a06f7fe... |
