fix: throw an error instead of ignoring it when uploading batches#207
fix: throw an error instead of ignoring it when uploading batches#207gasperzgonec wants to merge 4 commits into
Conversation
| const batch = this.items.slice(0, batchSize); | ||
| await this.upload(batch); | ||
| this.items.splice(0, batchSize); |
There was a problem hiding this comment.
This now first slices the items from the array, which doesn't remove them, uploads them, and if it fails, the items are still present in the array for processing in the next iteration.
There was a problem hiding this comment.
Okay, so in this case if upload fails it will throw an error which must be caught by developer if he wants and if not it will fail the sync, do I understand correctly?
There was a problem hiding this comment.
That's correct.
| for (const repo of this.repos) { | ||
| this.artifacts.push(...repo.uploadedArtifacts); | ||
| } |
There was a problem hiding this comment.
This was added, because until now, if we errored before now, the few artifacts that we uploaded, would have stayed without a parent, this sends them with the emit.
There was a problem hiding this comment.
Can you elaborate on this?
What is the exact issue? Maybe give an example?
There was a problem hiding this comment.
This is related to the S3 cleanup. I think that if we upload an artifact and we don't specify it in the event, to note that it was uploaded, it becomes an orphan and gets deleted after 30 days instead of when the sync is killed. Cleanup is deferred for the items that aren't reported.
| import { isMainThread, workerData } from 'node:worker_threads'; | ||
|
|
||
| /** Production default; tests may lower via workerData.options.httpRetries or ADAAS_TEST_HTTP_RETRIES. */ | ||
| function getHttpRetryCount(): number { | ||
| if ( | ||
| !isMainThread && | ||
| workerData?.options?.httpRetries !== undefined && | ||
| workerData.options.httpRetries >= 0 | ||
| ) { | ||
| return workerData.options.httpRetries; | ||
| } | ||
|
|
||
| const fromEnv = process.env.ADAAS_TEST_HTTP_RETRIES; | ||
| if (fromEnv !== undefined) { | ||
| const parsed = parseInt(fromEnv, 10); | ||
| if (!isNaN(parsed) && parsed >= 0) { | ||
| return parsed; | ||
| } | ||
| } | ||
| return 5; | ||
| } |
There was a problem hiding this comment.
This helps us extract the HTTP retry values from the environment or worker options.
radovanjorgic
changed the title
gasperzgonec
force-pushed
the
gasperz/ASCPT-28
branch
from
27ba842 to
7a4fc6e
Compare
There was a problem hiding this comment.
Why are these logger changes needed as part of this PR?
There was a problem hiding this comment.
They're not. I've removed them.
| for (const repo of this.repos) { | ||
| this.artifacts.push(...repo.uploadedArtifacts); | ||
| } |
There was a problem hiding this comment.
Can you elaborate on this?
What is the exact issue? Maybe give an example?
| /** | ||
| * Persists adapter state before emitting, except for stateless worker phases. | ||
| */ | ||
| private async postStateBeforeEmit( |
There was a problem hiding this comment.
This is somehting I have done in v2 as well. I just structured it a bit differently: beforeEmit, buildEmityPayload and afterEmit. What if we go in that direction?
| return true; | ||
| } | ||
|
|
||
| console.log( |
There was a problem hiding this comment.
This is something I wanted to do as part of another task but I imagine it is not a big change, can you log the state object here as well and also a state size if it is easy to calculate?
So this log looks sth like this: "Saving state before emitting event with event type . Current state {}". But if you decide to do this please test it properly both locally and on lambda (that logger formats it correctly).
| * Posts state (when required), emits to the platform, and notifies the parent | ||
| * worker that an event was emitted. | ||
| */ | ||
| private async emitToPlatform( |
There was a problem hiding this comment.
emitToPlatform is a bit bad naming if you ask me. Can you check what was done in v2? I moved the emit function from control-protocol.ts to emit.ts and reused it in adapters, emit in adapters is still called emit, with some before, build and after methods.
| const batch = this.items.slice(0, batchSize); | ||
| await this.upload(batch); | ||
| this.items.splice(0, batchSize); |
There was a problem hiding this comment.
Okay, so in this case if upload fails it will throw an error which must be caught by developer if he wants and if not it will fail the sync, do I understand correctly?
| * Applied to the slow Jest project only. Reduces axios-retry attempts so spawn | ||
| * integration tests finish in seconds instead of minutes. Not used in production. | ||
| */ | ||
| process.env.ADAAS_TEST_HTTP_RETRIES = '2'; |
There was a problem hiding this comment.
Please remove this optimization of axios client retries from this PR. I don't think this is the way to do it and we are changing the public interfaces this way as well, let's design this properly.
| workerPathOverrides?: WorkerPathOverrides; | ||
| skipConfirmation?: boolean; | ||
| /** Test-only: limits axios-retry attempts inside worker threads (see axios-client-internal). */ | ||
| httpRetries?: number; |
There was a problem hiding this comment.
As said above please remove related stuff (+ changes in axios client).
| options?: WorkerAdapterOptions; | ||
| resolve: (value: void | PromiseLike<void>) => void; | ||
| originalConsole?: Console; | ||
| logger?: Logger; |
There was a problem hiding this comment.
Why was this added?
2 participants
Description
Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.npm run lint.