Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2911193003:
Expose named caches as dimensions. (Closed)
DescriptionExpose named caches as dimensions.
The implementation has a lot to be desired but the assumptions are well
documented and the implementation is trivial. Refactor later if needed.
Include smoke test to confirm that it works as expected.
R=nodir@chromium.org
BUG=728127
Review-Url: https://codereview.chromium.org/2911193003
Committed: https://github.com/luci/luci-py/commit/6bdac1c6b9a57cdcd73d6b29ceba13a9547e2a40
Patch Set 1 : . #
Total comments: 3
Patch Set 2 : sort #Patch Set 3 : Use state.json instead #
Total comments: 2
Patch Set 4 : Remove os.getcwd() #Patch Set 5 : Fixed smoke test #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. R=nodir@chromium.org BUG= ========== to ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. BUG= ==========
maruel@chromium.org changed reviewers: + vadimsh@chromium.org
Description was changed from ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. BUG= ========== to ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. Include smoke test to confirm that it works as expected. R=nodir@chromium.org BUG= ==========
Description was changed from ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. Include smoke test to confirm that it works as expected. R=nodir@chromium.org BUG= ========== to ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. Include smoke test to confirm that it works as expected. R=nodir@chromium.org BUG=728127 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Tested on staging in addition to the smoke test. The 4 lines are horrible but I think I commented the assumptions well enough.
https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/os_utilities.py:536: d = os.path.join(os.getcwd(), u'c', u'named') "named" symlinks are best effort. Better not to put them to the critical path. Can we read c/state.json here instead? the code would be still simple
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/os_utilities.py:536: d = os.path.join(os.getcwd(), u'c', u'named') On 2017/05/31 16:52:06, nodir wrote: > "named" symlinks are best effort. Better not to put them to the critical path. Then maybe we shouldn't create them. :/ Either we ensure the symlinks are good when cleanup() is called, either we drop them, I prefer to not keep mostly-works paths. > Can we read c/state.json here instead? the code would be still simple Done.
lgtm https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2911193003/diff/40001/appengine/swarming/swar... appengine/swarming/swarming_bot/api/os_utilities.py:536: d = os.path.join(os.getcwd(), u'c', u'named') On 2017/05/31 20:45:32, M-A Ruel wrote: > On 2017/05/31 16:52:06, nodir wrote: > > "named" symlinks are best effort. Better not to put them to the critical path. > > Then maybe we shouldn't create them. :/ > Either we ensure the symlinks are good when cleanup() is called, either we drop > them, I prefer to not keep mostly-works paths. we keep them for troopers and alike. My point is that it is not fatal to fail to create a symlink, for example. > > > Can we read c/state.json here instead? the code would be still simple > > Done. https://codereview.chromium.org/2911193003/diff/100001/appengine/swarming/swa... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2911193003/diff/100001/appengine/swarming/swa... appengine/swarming/swarming_bot/api/os_utilities.py:537: with open(os.path.join(os.getcwd(), u'c', u'state.json'), 'rb') as f: is os.getcwd() needed? wouldn't open() do it for you?
https://codereview.chromium.org/2911193003/diff/100001/appengine/swarming/swa... File appengine/swarming/swarming_bot/api/os_utilities.py (right): https://codereview.chromium.org/2911193003/diff/100001/appengine/swarming/swa... appengine/swarming/swarming_bot/api/os_utilities.py:537: with open(os.path.join(os.getcwd(), u'c', u'state.json'), 'rb') as f: On 2017/05/31 21:03:21, nodir wrote: > is os.getcwd() needed? wouldn't open() do it for you? Removed, that's true that it was not necessary.
The CQ bit was checked by maruel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2911193003/#ps120001 (title: "Remove os.getcwd()")
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: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3683c5a76a80f710)
On 2017/06/02 22:39:43, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > Luci-py Presubmit on luci.infra.try (JOB_FAILED, > https://luci-milo.appspot.com/swarming/task/3683c5a76a80f710) Improved smoke test, no logic change.
The CQ bit was checked by maruel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2911193003/#ps140001 (title: "Fixed smoke test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496685642043120,
"parent_rev": "c3718286f7c918c30b898889f03eaa6c94a3ecbe", "commit_rev":
"6bdac1c6b9a57cdcd73d6b29ceba13a9547e2a40"}
Message was sent while issue was closed.
Description was changed from ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. Include smoke test to confirm that it works as expected. R=nodir@chromium.org BUG=728127 ========== to ========== Expose named caches as dimensions. The implementation has a lot to be desired but the assumptions are well documented and the implementation is trivial. Refactor later if needed. Include smoke test to confirm that it works as expected. R=nodir@chromium.org BUG=728127 Review-Url: https://codereview.chromium.org/2911193003 Committed: https://github.com/luci/luci-py/commit/6bdac1c6b9a57cdcd73d6b29ceba13a9547e2a40 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://github.com/luci/luci-py/commit/6bdac1c6b9a57cdcd73d6b29ceba13a9547e2a40 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
