Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2982763002:
Make mmutex actually acquire and release file locks (Closed)
|
Created:
3 years, 5 months ago by charliea (OOO until 10-5) Modified:
3 years, 4 months ago Reviewers:
iannucci CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
DescriptionMake mmutex actually acquire and release file locks
There's still some work to be done with customization and implementing
the shared command, but this CL makes the exclusive command functional.
BUG=416072
Review-Url: https://codereview.chromium.org/2982763002
Committed: https://github.com/luci/luci-go/commit/d3bd42031c01dff3209c39925428311e7a0249ff
Patch Set 1 #Patch Set 2 : Use gofslock instead of rolling own fslock #Patch Set 3 : Write tests for exclusive #
Total comments: 6
Patch Set 4 : #Patch Set 5 : Make the lock path Windows-friendly #Patch Set 6 : Merge branch 'master' into wrapper2 #Patch Set 7 : Use cross-platform touch command #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 55 (38 generated)
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
charliea@chromium.org changed reviewers: + iannucci@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Make mmutex actually acquire and release file locks There's still a fair amount of work to be done in terms of adding customizable timeouts and adding a shared command, and adding Windows support, but this CL does the bulk of the heavy lifting on Unix systems where we can now acquire and release a file lock via a synchronous call that respects a timeout parameter. BUG=416072 ========== to ========== y# Enter a description of the change. Make mmutex actually acquire and release file locks There's still some work to be done with customization and implementing the shared command, but this CL makes the exclusive command functional. BUG=416072 ==========
Description was changed from ========== y# Enter a description of the change. Make mmutex actually acquire and release file locks There's still some work to be done with customization and implementing the shared command, but this CL makes the exclusive command functional. BUG=416072 ========== to ========== Make mmutex actually acquire and release file locks There's still some work to be done with customization and implementing the shared command, but this CL makes the exclusive command functional. BUG=416072 ==========
charliea@chromium.org changed reviewers: + iannucci@chromium.org
PTAL
Patchset #3 (id:60001) has been deleted
lgtm https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/exclu... File mmutex/cmd/mmutex/exclusive.go (right): https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/exclu... mmutex/cmd/mmutex/exclusive.go:38: return 0 we return 0 if the error is NOT nil? should that be == above? https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.go File mmutex/cmd/mmutex/main.go (right): https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.... mmutex/cmd/mmutex/main.go:3: // that can be found in the LICENSE file. the copyrights should all look like the one on the left (the style changed in the luci-go repo ~this week) on all files https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.... mmutex/cmd/mmutex/main.go:16: var application = &subcommands.DefaultApplication{ note that subcommands lets you directly add documented environment variables: https://godoc.org/github.com/maruel/subcommands#DefaultApplication Which you could use for the lock file path.
https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/exclu... File mmutex/cmd/mmutex/exclusive.go (right): https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/exclu... mmutex/cmd/mmutex/exclusive.go:38: return 0 On 2017/07/14 17:53:47, iannucci wrote: > we return 0 if the error is NOT nil? should that be == above? Errors - how do they work? ¯\_(ツ)_/¯ https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.go File mmutex/cmd/mmutex/main.go (right): https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.... mmutex/cmd/mmutex/main.go:3: // that can be found in the LICENSE file. On 2017/07/14 17:53:47, iannucci wrote: > the copyrights should all look like the one on the left (the style changed in > the luci-go repo ~this week) on all files Done. https://codereview.chromium.org/2982763002/diff/80001/mmutex/cmd/mmutex/main.... mmutex/cmd/mmutex/main.go:16: var application = &subcommands.DefaultApplication{ On 2017/07/14 17:53:48, iannucci wrote: > note that subcommands lets you directly add documented environment variables: > https://godoc.org/github.com/maruel/subcommands#DefaultApplication > > Which you could use for the lock file path. Acknowledged.
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps100001 (title: " ")
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-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/375b1f4ac0d2e510)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps120001 (title: "Make the lock path Windows-friendly")
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-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/375b91bd67824310)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps140001 (title: "Merge branch 'master' into wrapper2")
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 charliea@chromium.org
Welp, that was dumb. I literally got rid of the part of the test that was testing my code...
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps160001 (title: "Use cross-platform touch command")
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-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/37947592cb43f710)
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps180001 (title: " ")
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 charliea@chromium.org
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps200001 (title: " ")
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-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3799310d0dbd3110)
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: Luci-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3799679dd8068b10)
The CQ bit was checked by charliea@chromium.org to run a CQ dry run
Dry run: 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 charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2982763002/#ps260001 (title: " ")
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": 260001, "attempt_start_ts": 1501103067749520,
"parent_rev": "8ccb74d02ea1ac77c1bff3a6230d7e8dd2280817", "commit_rev":
"d3bd42031c01dff3209c39925428311e7a0249ff"}
Message was sent while issue was closed.
Description was changed from ========== Make mmutex actually acquire and release file locks There's still some work to be done with customization and implementing the shared command, but this CL makes the exclusive command functional. BUG=416072 ========== to ========== Make mmutex actually acquire and release file locks There's still some work to be done with customization and implementing the shared command, but this CL makes the exclusive command functional. BUG=416072 Review-Url: https://codereview.chromium.org/2982763002 Committed: https://github.com/luci/luci-go/commit/d3bd42031c01dff3209c39925428311e7a0249ff ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as https://github.com/luci/luci-go/commit/d3bd42031c01dff3209c39925428311e7a0249ff |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
