refactor: rewrite in node for less tool clutter #7
Conversation
m90
changed the title
| await execCmd(process.env.CALLBACK_ON_FAILURE) | ||
| } | ||
| }) | ||
| .catch((err) => { |
There was a problem hiding this comment.
Interesting question: if the transfer itself worked, but the callback failed, the script will now still exit 0. Is this what we want? I'm unsure.
There was a problem hiding this comment.
After trying to think through the steps that would happen in this case I believe it's probably no worse than returning 1.
In either case because of how we specified the spec the job won't retry.
This means that it may be marginally less clear to the debugging operator but on the other hand the "main work" done by the job did succeed.
| process.exitCode = 1 | ||
| }) | ||
| .then(() => { | ||
| if (process.env.CALLBACK_ON_SUCCESS && process.exitCode === 0) { |
There was a problem hiding this comment.
Running the callbacks from in here now means we don't catch SIGINT, but I guess that's ok for our use case?
There was a problem hiding this comment.
I'm not sure if this might leave us with a larger (but still small) chance of wiki's being left in a partially imported state. In any case I don't think k8s is likely to SIGINT but rather SIGTERM. If we wanted to be super belt and braces we could trap this and then CALLBACK_ON_FAILURE
There was a problem hiding this comment.
There is a cleanup Laravel job that sweeps stalled imports every hour or so.
We could leave the trap in bash as is if we wanted to, but then we'd have a bit of mish-mash still. Which is what we are trying to leave behind.
There was a problem hiding this comment.
Added some Node based trapping in 214be4d that seems to work when double CRL+Cing the thing running in Docker
➜ transferbot git:(fr/nodeify) ✗ docker run -e TARGET_WIKI_OAUTH_CONSUMER_TOKEN="xxx" -e TARGET_WIKI_OAUTH_CONSUMER_SECRET="xxx" -e TARGET_WIKI_OAUTH_ACCESS_TOKEN="xxx" -e TARGET_WIKI_OAUTH_ACCESS_SECRET="xxx" -e CALLBACK_ON_FAILURE="echo 'failing'" --rm 4ab54cf0400349e5e0afbf06e1965046c1b7cf0f8890c505afd8b733efac3a07 https://www.wikidata.org https://updatewiki.wikibase.dev Q107819273
^CAn error occurred executing the command:
Error: Received SIGINT, program will exit before finishing.
at process.<anonymous> (file:///pkg/lib/signals.js:5:16)
at process.emit (node:events:519:28)
failing
^C%
Single Ctrl+C would have Node waiting for its event loop to become empty and would trigger both the transfer as well as the failure callback. I would think this is a problem we'd also have in bash.
There was a problem hiding this comment.
I ended up needing to do a load of reading the docs about process and this then inspired me to ask: "should we consider using beforeExit for all this stuff?" see https://nodejs.org/api/process.html#event-beforeexit
There was a problem hiding this comment.
So this is a bit ugly. When using beforeExit, a forceful exit (i.e. Ctrl+C) or an unhandled exception will not be considered:
The 'beforeExit' event is not emitted for conditions causing explicit termination, such as calling process.exit() or uncaught exceptions.
In theory on('exit') would help us, however this (as opposed to beforeExit) only allows for synchronous code to be run in the callback:
Listener functions must only perform synchronous operations. The Node.js process will exit immediately after calling the 'exit' event listeners causing any additional work still queued in the event loop to be abandoned.
meaning we can't shell out to the command the user is passing.
Maybe we're already at a point where this is "as good as it gets" if we want to keep this in Node.
There was a problem hiding this comment.
Maybe it's good to remind ourselves to not overcomplicate this and aim for a solution that ensures the given commands will ALWAYS run as expected.
There was a problem hiding this comment.
I implemented some cancellation mechanism in 49f77f1
This works as intended now, but I am not 100% sure we should use it as it makes the code kind of hard to follow. This is mostly because the wikibase-x librarys do not expose the fetch they are using, so we cannot use the plain signal but have to add some custom wrapper, when fetch calls can use the signal as intended. I'm not really sure if the extra complexity justifies the outcome.
m90
force-pushed
the
fr/nodeify
branch
4 times, most recently
from
54e0dbf to
6444b66
Compare
lib/repo.js
Outdated
| return Promise.reject(new Error('Cannot edit a read only instance.')) | ||
| } | ||
| return Promise.all(entities.map(async entity => { | ||
| return this.edit.entity.create(entity) |
There was a problem hiding this comment.
Does this stop the whole import if one entity fails to be created? I think I've convinced myself that we would rather have this continue even if one entity fails
There was a problem hiding this comment.
This rejects when the first promise in the given array rejects, so it's not entirely predictable how much work is done in such a case. We should be able to use allSettled though and get what you describe. Question though is: do we consider such a case a failure still and exit 1?
There was a problem hiding this comment.
nice; part of my reasoning is that it means rerunning the job will be less painful in future (although the rerun will also error).
| import { execCmd } from './../lib/exec.js' | ||
| import { trap } from './../lib/signals.js' | ||
|
|
||
| Promise.race([ |
There was a problem hiding this comment.
praise: I needed to quickly look up how this fancy thing worked and that's very cool. Looks suitably tidy!
|
I'm not finished reviewing this; I really wanted to "just try it" but need to look some more tomorrow about how to best actually do this. We're you developing by building the image and then adding it into minikube? Or using skaffold? Or building it locally and running locally after exfiltrating credentials from your local development environment? Or even something else? |
This is how I tested it, yes. |
|
Looking at this in its current state I have to admit I would lean towards not merging this, as it is now FAR more complex than what we had, even if it's just using a single language. To me, this is a bad tradeoff. But since I am not the person who'll be maintaining this, I'll leave the decision up to you. |
|
As discussed, this is not what we want as it makes things more complex instead of giving us simplicity. |
As discussed, the current state using bash, Node and Python for a pretty small piece of software might be detrimental for long term maintainability of this thing.