Refactor x/lockfile into dbnode/server #2862
Conversation
scripts/lockfile/lockfile.go
Outdated
| // This .go file is used to test the lockfile package (m3/src/x/lockfile) | ||
| // This .go file is used to test the lockfile implementation in dbnode/server. | ||
| // The implementation is internal to dbnode/server, but is utilized by this script | ||
| // so that its test can exec new processes that acquire file locks. |
There was a problem hiding this comment.
lockfile_test.go execs this script to intentionally spin up a new process. The script accordingly runs in a main package, meaning we're not able to make the APIs internal
wesleyk
force-pushed
the
wesley-lockfile
branch
from
1c53585
to
f7d347f
Compare
wesleyk
force-pushed
the
wesley-lockfile
branch
from
f7d347f
to
51d2435
Compare
| @@ -57,7 +57,7 @@ func Acquire(path string) (*Lockfile, error) { | |||
| } | |||
|
|
|||
| // CreateAndAcquire creates any non-existing directories needed to | |||
| // create the lock file, then acquires a lock on it | |||
| // create the lock file, then acquires a lock on it. | |||
There was a problem hiding this comment.
we'll want to rename these to AcquireLockfile, CreateAndAcquireLockfile, etc (or similar); the general notion of "acquiring" is ambiguous in the context of a server package.
scripts/lockfile/lockfile.go
Outdated
| @@ -20,7 +20,9 @@ | |||
|
|
|||
| package main | |||
|
|
|||
| // This .go file is used to test the lockfile package (m3/src/x/lockfile) | |||
| // This .go file is used to test the lockfile implementation in dbnode/server. | |||
There was a problem hiding this comment.
I wonder if there's a better way for this to work - we're basically forced to export lockfile types from server simply because we spin up a new process here, where it would be instead preferable to not export lockfiles from server because they're not used anywhere else.
One option would be moving lockfiles into server/internal/lockfile (which would also let us keep Acquire, CreateAndAcquire verbatim) and then have server/internal/acquire-lock as a main package for spinning up processes, so we keep things unexported outside of an internal package. Thoughts?
src/dbnode/server/lockfile_test.go
Outdated
| func TestAcquireAndReleaseFile(t *testing.T) { | ||
| // immediately return if this test wasn't invoked by another test in the | ||
| if os.Getenv("LOCKFILE_SUPERVISED_PROCESS") != "true" { | ||
| return |
There was a problem hiding this comment.
nit: let's do a t.Skip() here instead
src/dbnode/server/lockfile_test.go
Outdated
| lockPath := os.Getenv("WITH_LOCK_PATH") | ||
| removeLock := os.Getenv("WITH_REMOVE_LOCK") | ||
| sleepDuration := os.Getenv("WITH_SLEEP_DURATION") |
There was a problem hiding this comment.
style nit:
var (
lockPath = ...
removeLock = ...
sleepDuration = ...
)
| // Lockfile represents an acquired lockfile. | ||
| type Lockfile struct { | ||
| // lockfile represents an acquired lockfile. | ||
| type lockfile struct { |
There was a problem hiding this comment.
Thanks to @robskillington's idea to invoke tests as a separate process rather than a go cmd!
wesleyk
force-pushed
the
wesley-lockfile
branch
from
ba95e5a
to
225aa18
Compare
wesleyk
force-pushed
the
wesley-lockfile
branch
from
5f4469f
to
01cfc46
Compare
* master: (28 commits) [dbnode] Add claims for index segments volume index (#2846) [dbnode] Remove namespaces from example config and integration tests (#2866) [dbnode] Resurrect flaky test skip (#2868) [aggregator] Fix checkCampaignStateLoop (#2867) [dbnode] implement deletion method in namespace kvadmin service (#2861) Replace closer with resource package (#2864) Add coding style guide (#2831) Add GOVERNANCE.md to describe governance (#2830) Add COMPATIBILITY.md to describe version compatibility (#2829) Refactor etcd config as discovery section with convenience types (#2843) Refactor x/lockfile into dbnode/server (#2862) [lint] Disable nlreturn linter (#2865) [m3cluster] Expose placement algorithm in placement service (#2858) [etcd] Set reasonable cluster connection/sync settings by default (#2860) [dbnode] Use bits.LeadingZeros64 to improve encoder performance (#2857) Cleanup m3nsch leftovers (#2856) Update ci-scripts to correct coverage tracking (#2854) [aggregator] Process campaign state without waiting for first campaign check interval (#2855) Bump go to 1.14 (#2853) [query] Remove single series error from M3 ...
What this PR does / why we need it:
Moves lockfile.go locally to its only usage in dbnode/server.
Note that there's an additional usage in
scripts/lockfile/lockfile.gothat requires the API to remain public to test the code out.Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE
Does this PR require updating code package or user-facing documentation?:
NONE