Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9978016:
Modify the sharding supervisor to support sharding only a sub-shard of a test suite. (Closed)
|
Created:
8 years, 8 months ago by Nicolas Sylvain Modified:
8 years, 8 months ago CC:
chromium-reviews, csharp Visibility:
Public. |
DescriptionModify the sharding supervisor to support sharding only a sub-shard of a test suite.
This is required if we want to enable sharding on the main waterfall.
TEST=sharding_supervisor_unittest.py
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131275
Patch Set 1 #
Total comments: 19
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 3
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py File sharding_supervisor.py (right): https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:182: class ShardingSupervisor(object): not sure where to add the docs, here or below, but somewhere please document the relationship between your slices/slots/etc, # of ShardRunners(), and the GTest suites they run with various "SHARD" settings https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:199: suite_total_shards: Total number of shards the suite is split into if we we agreed that suite_*_shards and suite_shard_* could be replaced with better names https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:466: help="number of shards when running only a subset of the suite") change to 'if running a subset, ...' https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:469: help="index of the shard to run when running only a subset of the suite") change to 'if running a subset, ...' https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... File sharding_supervisor_unittest.py (right): https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:12: import re sort in alphabetic order https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:19: can this line be removed? https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:21: SHARDS_PER_CORE = sharding_supervisor.SS_DEFAULT_SHARDS_PER_CORE can these be sorted in order? https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:45: self.assertEqual(p.returncode, 0) to stay consistent, let's put 0 on the left side and p.returncode on the right https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:59: self.assertEqual(p.returncode, 0) same as above https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:81: self.assertEqual(p.returncode, 0) same as above
updated https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py File sharding_supervisor.py (right): https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:182: class ShardingSupervisor(object): On 2012/04/04 23:00:28, cmp wrote: > not sure where to add the docs, here or below, but somewhere please document the > relationship between your slices/slots/etc, # of ShardRunners(), and the GTest > suites they run with various "SHARD" settings Done. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:199: suite_total_shards: Total number of shards the suite is split into if we On 2012/04/04 23:00:28, cmp wrote: > we agreed that suite_*_shards and suite_shard_* could be replaced with better > names Done. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor.py#... sharding_supervisor.py:466: help="number of shards when running only a subset of the suite") On 2012/04/04 23:00:28, cmp wrote: > change to 'if running a subset, ...' rewrote that, but still not happy. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... File sharding_supervisor_unittest.py (right): https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:12: import re On 2012/04/04 23:00:28, cmp wrote: > sort in alphabetic order Gotta talk to the guy who wrote the code I stole this from ;) https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:19: On 2012/04/04 23:00:28, cmp wrote: > can this line be removed? Done. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:21: SHARDS_PER_CORE = sharding_supervisor.SS_DEFAULT_SHARDS_PER_CORE On 2012/04/04 23:00:28, cmp wrote: > can these be sorted in order? what order? https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:45: self.assertEqual(p.returncode, 0) On 2012/04/04 23:00:28, cmp wrote: > to stay consistent, let's put 0 on the left side and p.returncode on the right Done. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:59: self.assertEqual(p.returncode, 0) On 2012/04/04 23:00:28, cmp wrote: > same as above Done. https://chromiumcodereview.appspot.com/9978016/diff/1/sharding_supervisor_uni... sharding_supervisor_unittest.py:81: self.assertEqual(p.returncode, 0) On 2012/04/04 23:00:28, cmp wrote: > same as above Done.
Sorry for the nitpickingfest. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor.py File sharding_supervisor.py (right): https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor.... sharding_supervisor.py:45: SS_DEFAULT_TOTAL_SLAVES = 1 # run the whole suite. I don't think these 2 constants are useful. That feels like brain-dead copy paste to me. :) https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... File sharding_supervisor_unittest.py (right): https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:20: 2 lines. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:21: def GenerateExpectedOutput(start, end, num_shards): In general, I prefer new code to use PEP8 names. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:24: for i in range(start, end): If I were picky, I'd tell you to use xrange(). You can ask Stefan how much he likes to be told that. :) https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:25: stdout += 'Running shard %d of %d\n' % (i, num_shards) stdout = ''.join( 'Running shard %d of %d\n' % (i, num_shards) for i in xrange(start, end)) https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:35: (expected_out, expected_err) = GenerateExpectedOutput(0, expected_shards, This kind of alignment makes me sad. :/ https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:40: p.wait(); ; ? p.wait()? See http://docs.python.org/library/subprocess.html#subprocess.Popen.wait
+ csharp since this CL is needed to support sharding_supervisor due to the 2 levels of sharding.
updated https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor.py File sharding_supervisor.py (right): https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor.... sharding_supervisor.py:45: SS_DEFAULT_TOTAL_SLAVES = 1 # run the whole suite. On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > I don't think these 2 constants are useful. That feels like brain-dead copy > paste to me. :) That's the style of the file. We could probably remove them all, but for some reasons they are all here. Not really worth changing the style right now. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... File sharding_supervisor_unittest.py (right): https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:20: On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > 2 lines. Done. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:21: def GenerateExpectedOutput(start, end, num_shards): On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > In general, I prefer new code to use PEP8 names. Done. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:24: for i in range(start, end): On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > If I were picky, I'd tell you to use xrange(). You can ask Stefan how much he > likes to be told that. :) Good thing you are not picky. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:25: stdout += 'Running shard %d of %d\n' % (i, num_shards) On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > stdout = ''.join( > 'Running shard %d of %d\n' % (i, num_shards) > for i in xrange(start, end)) I actually don't find this code more readable and it's the same number of lines and something like 5 more chars. https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:35: (expected_out, expected_err) = GenerateExpectedOutput(0, expected_shards, On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > This kind of alignment makes me sad. :/ what about this one? https://chromiumcodereview.appspot.com/9978016/diff/5001/sharding_supervisor_... sharding_supervisor_unittest.py:40: p.wait(); On 2012/04/05 00:53:58, Marc-Antoine Ruel wrote: > ; ? > > p.wait()? > See http://docs.python.org/library/subprocess.html#subprocess.Popen.wait Done.
lgtm
lgtm https://chromiumcodereview.appspot.com/9978016/diff/10001/sharding_supervisor.py File sharding_supervisor.py (right): https://chromiumcodereview.appspot.com/9978016/diff/10001/sharding_supervisor... sharding_supervisor.py:45: SS_DEFAULT_TOTAL_SLAVES = 1 # run the whole suite. yes! https://chromiumcodereview.appspot.com/9978016/diff/10001/sharding_supervisor... sharding_supervisor.py:71: def RunShard(test, total_shards, index, gtest_args, stdout, stderr): thank you! https://chromiumcodereview.appspot.com/9978016/diff/10001/sharding_supervisor... sharding_supervisor.py:211: shards [0-19] or shards [20-39] depending if you set slave_index to 0 or 1. will help a lot, thanks
hi, I believe this broke whatever output the flakiness dashboard was depending on. see for ex: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40... it looks like the data stopped coming in with this revision
On 2012/04/12 23:00:55, John Abd-El-Malek wrote: > hi, I believe this broke whatever output the flakiness dashboard was depending > on. see for ex: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...) > > it looks like the data stopped coming in with this revision try this url instead: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
hmm, link isn't going through rietveld correctly, try this: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40... On Thu, Apr 12, 2012 at 4:07 PM, <jam@chromium.org> wrote: > On 2012/04/12 23:00:55, John Abd-El-Malek wrote: > >> hi, I believe this broke whatever output the flakiness dashboard was >> depending >> on. see for ex: >> > > http://test-results.appspot.**com/dashboards/flakiness_** > dashboard.html#group=%**2540DEPS%2520-%2520chromium.** > org&testType=browser_tests&**sortOrder=forward&sortColumn=** > test&showExpectations=true&**builder=Linux%2520Tests%2520%** > 28dbg%29%28shared<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%2540DEPS%2520-%2520chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%2520Tests%2520%28dbg%29%28shared> > ) > > > it looks like the data stopped coming in with this revision >> > > try this url instead: > > http://test-results.appspot.**com/dashboards/flakiness_** > dashboard.html#group=%40DEPS%**20-%20chromium.org&testType=** > browser_tests&sortOrder=**forward&sortColumn=test&** > showExpectations=true&builder=**Linux%20Tests%20(dbg)(shared)<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40DEPS%20-%20chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%20Tests%20(dbg)(shared)> > > https://chromiumcodereview.**appspot.com/9978016/<https://chromiumcodereview.... >
thanks for letting me know. I'll take a look On Thu, Apr 12, 2012 at 4:08 PM, John Abd-El-Malek <jam@chromium.org> wrote: > hmm, link isn't going through rietveld correctly, try this: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40... > > > > On Thu, Apr 12, 2012 at 4:07 PM, <jam@chromium.org> wrote: > >> On 2012/04/12 23:00:55, John Abd-El-Malek wrote: >> >>> hi, I believe this broke whatever output the flakiness dashboard was >>> depending >>> on. see for ex: >>> >> >> http://test-results.appspot.**com/dashboards/flakiness_** >> dashboard.html#group=%**2540DEPS%2520-%2520chromium.** >> org&testType=browser_tests&**sortOrder=forward&sortColumn=** >> test&showExpectations=true&**builder=Linux%2520Tests%2520%** >> 28dbg%29%28shared<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%2540DEPS%2520-%2520chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%2520Tests%2520%28dbg%29%28shared> >> ) >> >> >> it looks like the data stopped coming in with this revision >>> >> >> try this url instead: >> >> http://test-results.appspot.**com/dashboards/flakiness_** >> dashboard.html#group=%40DEPS%**20-%20chromium.org&testType=** >> browser_tests&sortOrder=**forward&sortColumn=test&** >> showExpectations=true&builder=**Linux%20Tests%20(dbg)(shared)<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40DEPS%20-%20chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%20Tests%20(dbg)(shared)> >> >> https://chromiumcodereview.**appspot.com/9978016/<https://chromiumcodereview.... >> > >
On Thu, Apr 12, 2012 at 4:32 PM, Nicolas Sylvain <nsylvain@google.com>wrote: > thanks for letting me know. I'll take a look > > > On Thu, Apr 12, 2012 at 4:08 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> hmm, link isn't going through rietveld correctly, try this: >> >> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40... >> >> >> this link now works. I think I fixed the issues, but there might still be broken parts. let me know if you see something weird. thanks Nicolas > >> On Thu, Apr 12, 2012 at 4:07 PM, <jam@chromium.org> wrote: >> >>> On 2012/04/12 23:00:55, John Abd-El-Malek wrote: >>> >>>> hi, I believe this broke whatever output the flakiness dashboard was >>>> depending >>>> on. see for ex: >>>> >>> >>> http://test-results.appspot.**com/dashboards/flakiness_** >>> dashboard.html#group=%**2540DEPS%2520-%2520chromium.** >>> org&testType=browser_tests&**sortOrder=forward&sortColumn=** >>> test&showExpectations=true&**builder=Linux%2520Tests%2520%** >>> 28dbg%29%28shared<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%2540DEPS%2520-%2520chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%2520Tests%2520%28dbg%29%28shared> >>> ) >>> >>> >>> it looks like the data stopped coming in with this revision >>>> >>> >>> try this url instead: >>> >>> http://test-results.appspot.**com/dashboards/flakiness_** >>> dashboard.html#group=%40DEPS%**20-%20chromium.org&testType=** >>> browser_tests&sortOrder=**forward&sortColumn=test&** >>> showExpectations=true&builder=**Linux%20Tests%20(dbg)(shared)<http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40DEPS%20-%20chromium.org&testType=browser_tests&sortOrder=forward&sortColumn=test&showExpectations=true&builder=Linux%20Tests%20(dbg)(shared)> >>> >>> https://chromiumcodereview.**appspot.com/9978016/<https://chromiumcodereview.... >>> >> >> > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
