fix: Improve Lock Manager HTTP layer #15
Conversation
README.md
Outdated
| @@ -408,7 +423,7 @@ Similarly run git server for "remote test app" | |||
| scripts/host-project-in-git.sh /tmp/remote-sample $(pwd)/../examples/graph-perf-test-app | |||
| ``` | |||
|
|
|||
| Please note that when these projects will be feetched from local git by `k8-deployer`, the fetched project will have no `.env` file! However, our deployment scripts under `deployment/pit/*` can locate `.env` file by reading global environment variables. This is intended for local development. Below is the list of global environment variables | |||
| Please note that when these projects will be feetched from local git by `k8-deployer`, the fetched project will have no `.env` file! However, our deployment scripts under `deployment/pit/*` can locate `.env` file by reading global environment variables. This is intended for local development. Below is the list of global environment variables | |||
There was a problem hiding this comment.
Small typo feetched -> fetched
| } | ||
|
|
||
| const result = await db.execute(query) | ||
|
|
||
| let unlockedKeys = result?.rows?.map(({ lock_id }) => lock_id) | ||
| if (unlockedKeys?.length === 0) { | ||
| throw new Error("release(): No valid lock and owner combination found in database to delete") | ||
| throw new Error("No valid lock and owner combination found in database to delete") |
There was a problem hiding this comment.
Question: Should this be treated as an error?
There was a problem hiding this comment.
It does not matter as long as caller knows what to do. For example if user1 not owning a lock but trying to release it, this will produce error because "user1 + lockid" is not known. Similar with keep alive. service. Using errors gives us just 2 states: error or success. If we do not want to use error then we ned three states, which is harder to code maybe :)
There was a problem hiding this comment.
I'd say that releasing a previously held lock (by deleting an existing record) and having no lock to release yield the same outcomes for the caller. They are effectively idempotent. This shouldn't be treated as an error.
| logger.warn("http-client.invoke(): attempt %s of %s invoking %s. Error: %s", attempts, options.retries, JSON.stringify(options.fetchParams), error) | ||
| if (attempts < options.retries) { | ||
| attempts++ | ||
| await sleep(options.retryDelay) |
There was a problem hiding this comment.
Because we have this sleep in the invoke method, maybe we should remove this sleep while trying to acquire lock, else we are going to sleep longer duration than necessary.
There was a problem hiding this comment.
Yes, makes sense to remove the second one.
fmarek-kindred
force-pushed
the
feature/lock-manager-work
branch
from
fc41546 to
f0cbde3
Compare
No description provided.