close connections properly #14
Conversation
ledhed2222
force-pushed
the
close-connections
branch
from
8dbf31d
to
dcd34d4
Compare
ledhed2222
force-pushed
the
close-connections
branch
from
dcd34d4
to
4a24b15
Compare
index.js
Outdated
|
|
||
| Client.ready().then(Connection => { | ||
| Client.ready().then(() => { |
There was a problem hiding this comment.
removed this since I know understand that Client.ready returns Client anyway and we have that variable in global scope here. fewer variables is easier to read.
index.js
Outdated
| }) | ||
| .catch(err => { | ||
| if (err && err.name === 'PartialFailureError') { | ||
| if (err.errors && err.errors.length > 0) { | ||
| console.log('Insert errors:') | ||
| err.errors.forEach(err => console.dir(err, { depth: null })) | ||
| process.exit(1) | ||
| return safeHalt() |
There was a problem hiding this comment.
FYI - I personally do not like .then and greatly prefer async/await. It took me forever to remember how to await a promise from within another promise, conditionally, without nesting. to me, the fact that returning a promise from then will await the returned promise is too magical. async/await is much clearer
There was a problem hiding this comment.
have we considered using util.promisify to wrap this callback function bigquery.dataset(DATASET_NAME).table(TRANSACTION_TABLE_NAME).insert portion?
There was a problem hiding this comment.
instead of defining safeHalt and needing to catch every possible exception and call it, have you considered using a higher level error handling in node? I think defining a process.on('uncaughtException' ... and process.on('unhandledRejection' can allow you to catch these errors on the high level and not need to do a bunch of .then code clauses on callbacks that just call safeHalt
index.js
Outdated
| }) | ||
| .catch(err => { | ||
| if (err && err.name === 'PartialFailureError') { | ||
| if (err.errors && err.errors.length > 0) { | ||
| console.log('Insert errors:') | ||
| err.errors.forEach(err => console.dir(err, { depth: null })) | ||
| process.exit(1) | ||
| return safeHalt() |
There was a problem hiding this comment.
have we considered using util.promisify to wrap this callback function bigquery.dataset(DATASET_NAME).table(TRANSACTION_TABLE_NAME).insert portion?
ledhed2222
force-pushed
the
close-connections
branch
from
23fa88b
to
390db6d
Compare
ledhed2222
force-pushed
the
close-connections
branch
from
a9b9bad
to
df1cd95
Compare
index.js
Outdated
| const txPromises = result.ledger.transactions.map((tx) => { | ||
| return client.send({ | ||
| command: 'tx', | ||
| transaction: tx, | ||
| }, xrplRequestOptions) | ||
| }) |
There was a problem hiding this comment.
do you potentially run into a rate limit here if you're sending a ton of client requests at the same time?
There was a problem hiding this comment.
if you were using the promise-utils lib i linked you earlier, a mapLimit could be used to limit the number of parallel requests happening (https://blend.github.io/promise-utils/latest/index.html#maplimit)
Properly close connections and use timeouts for requests so that we actually quit when we're rate-limited and something like
systemdorforevercan handle instead of just spinning.this took a while to figure out (it was the timeouts with xrpl-client). i had to refactor everything to use
asyncto even really understand what was going on.you'll definitely want to review using "split mode", since I completely rewrote the
index.jsandledgerIndex.jsscripts