fix(executor): added error when no proposals are received #607
Conversation
JST-462: Added startupTimeout (default 30sec) as TaskExecutor option, after which if there is no at least one proposal we will get an appropriate error
mgordel
requested review from
grisha87,
SewerynKras,
pgrzy-golem and
cryptobench
src/executor/executor.ts
Outdated
| @@ -54,6 +54,8 @@ export type ExecutorOptions = { | |||
| * For more details see {@link JobStorage}. Defaults to a simple in-memory storage. | |||
| */ | |||
| jobStorage?: JobStorage; | |||
| /** Timeout for waiting for at least one offer from the market */ | |||
There was a problem hiding this comment.
Please provide a more user friendly description in the second line, explaining why the user should care and what they can do with this setting, what will happen when the timeout is reached. Documenting it here is far more important than writing the article in the docs :)
There was a problem hiding this comment.
you're right, I'll add a better description
src/executor/executor.ts
Outdated
| @@ -385,11 +390,29 @@ export class TaskExecutor { | |||
| timeout: options?.timeout ?? this.options.taskTimeout, | |||
| }); | |||
| this.taskQueue.addToEnd(task as Task<unknown, unknown>); | |||
| if (!this.startupTimeoutError && !this.startupTimeoutId) { | |||
There was a problem hiding this comment.
I have few issues with this, we can talk about this f2f:
- This way of communicating the error with the timeout isn't easy for the user to handle, we should work on that
- Since this is implemented as part of
executeTask, this timeout will be started and counted for each scheduled task. I do see a concurrency problem here, given all the tasks are executed on the same TaskExecutor instance.
There was a problem hiding this comment.
this timeout will only start once, unless this.startupTimeoutId exists yet, so there shouldn't be a problem with concurrency, I think, but let's talk about it..
src/market/demand.ts
Outdated
| @@ -27,6 +27,7 @@ export interface DemandOptions { | |||
| offerFetchingInterval?: number; | |||
| proposalTimeout?: number; | |||
| eventTarget?: EventTarget; | |||
| startupTimeout?: number; | |||
There was a problem hiding this comment.
This field should be documented as well.
There was a problem hiding this comment.
this is unnecessary, left by mistake, i will delete it
src/market/service.ts
Outdated
| @@ -28,6 +27,12 @@ export class MarketService { | |||
| private logger?: Logger; | |||
| private taskPackage?: Package; | |||
| private maxResubscribeRetries = 5; | |||
| private proposalsCount = { | |||
| initial: 0, | |||
| filtered: 0, | |||
There was a problem hiding this comment.
Filtered means accepted or rejected? The naming is confusing.
There was a problem hiding this comment.
filtered is not used, it was left by mistake
…posal-warning # Conflicts: # src/executor/executor.ts
fixes in readme
|
🎉 This PR is included in version 0.12.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 0.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
JST-462: Added startupTimeout (default 30sec) as TaskExecutor option, after which if there is no at least one proposal we will get an appropriate error