Queue artifacts table, phase 2, step 2 #3152
Conversation
helfi92
force-pushed
the
port_queue_artifacts_step_2
branch
2 times, most recently
from
2492292
to
6abfb25
Compare
db/versions/0017.yml
Outdated
| @@ -229,3 +229,152 @@ methods: | |||
| end; | |||
| end if; | |||
| end | |||
| delete_queue_artifact: | |||
There was a problem hiding this comment.
You'll need to move these to a new DB version, since they'll land in a later release than step 1.
services/queue/src/utils.js
Outdated
|
|
||
| const fetchResults = async (continuation) => { | ||
| const query = continuation ? { continuationToken: continuation } : {}; | ||
| const {continuationToken, rows} = await paginateResults({ |
There was a problem hiding this comment.
paginateResults is for APIs.. it's just abstracting over the pagination in a way that obscures what's really happening here. I think this function could just loop with adjustments to page_offset on each iteration, until it gets zero results.
services/queue/src/utils.js
Outdated
| */ | ||
| async expire({ db, publicBucket, privateBucket, ignoreError, monitor, expires }) { | ||
| let count = 0; | ||
| await exports.artifactUtils.getExpiredArtifacts(db, expires, async (artifact) => { |
There was a problem hiding this comment.
This seems to be the only use of getExpiredArtifacts, so perhaps that utility could be combined here, or incorporated as a nested function?
I think this is going to miss expiring artifacts:
- call get_queue_artifacts with a page size of 1000
- call S3 for each of those 1000
- call delete_artifact for each of those 1000
- call get_queue_artifacts with a page size of 1000, offset 1000
..at this point, we've missed 1000 expired artifacts since the offset skips 1000 non-deleted artifacts.
This actually isn't the end of the world -- those artifacts would hopefully just get picked up in the next run of the expiration job -- but might be confusing when we wonder why there's a small tail of expired artifacts in the DB.
I suspect the fix would be to decrement the offset by 1 for every call to delete_queue_artifact. In the success case, that will end up calling get_queue_artifacts with offset = 0 every time, until it returns no results, but in the failure case it will not re-try deleting S3 objects.
Separately, it seems like we should be taking a closer look at these errors instead of just ignoring them. I'll file a followup for that.
| artifact.taskId, artifact.runId, artifact.name, details, | ||
| expires, artifact.etag | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Since this passes etag, it should handle conflicts. I'm not sure it needs to handle the etag, though? I think this is just for idempotency so we can assume that this call will override a previous call.
And, with that change, you can drop etag support for update_queue_artifact, which simplifies its implementation quite a bit.
helfi92
force-pushed
the
port_queue_artifacts_step_2
branch
from
bdea363
to
97f03f0
Compare
helfi92
force-pushed
the
port_queue_artifacts_step_2
branch
from
97f03f0
to
d0f0b0e
Compare
helfi92
force-pushed
the
port_queue_artifacts_step_2
branch
from
d0f0b0e
to
e8a5986
Compare
No description provided.