Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Delay KV bulk delete #1115

Merged
merged 13 commits into from
Mar 11, 2020
Merged

Delay KV bulk delete #1115

merged 13 commits into from
Mar 11, 2020

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented Mar 2, 2020

Overview

Fixes #783

Previously, we would run a full KV namespace sync BEFORE uploading the worker script to the edge. This caused some unacceptable downtime for updated assets, as the Worker would continue handling requests to paths with outdated KV keys. This PR does a few things in service of re-ordering these operations, but the bottom line is that now on Wrangler publish, when the project is a Workers Site, we:

  1. validate the wrangler.toml
  2. run wrangler build
  3. query the site's KV namespace for existing keys and diff them against the local site bucket to build the list of files to upload, the list of keys to delete, and the asset manifest to be uploaded with the Worker script.
  4. execute the bulk file upload
  5. upload the worker with its new asset manifest
  6. execute the bulk file delete

This has the added benefit of NOT deleting old values in the case where the Worker upload fails, since the first KV operation is additive.

Things this is not

This PR introduces a little more verbosity into preview/upload and publish, however there is considerable overlap between those operations already, but I'd like to address those in a separate PR in order to expedite this fix.

I'm also interested in optimizing the preview command's behavior (refrain from deleting until a ctrl-C or something like that) but not here.

💅

There's a few house-keeping changes here, though more remain which I'll include as inline comments:

  • we now pass a set of KeyValuePairs to kv::bucket::upload, rather than a bucket.
  • some house-keeping in the publish command, mostly renaming and some refactors
  • remove unused/unsupported bucket attribute from KvNamespace struct

@ashleymichal ashleymichal added status - needs review regression Something is broken, but works in previous releases sites labels Mar 2, 2020
@ashleymichal ashleymichal added this to the sites clean up milestone Mar 2, 2020
@ashleymichal ashleymichal requested a review from a team March 2, 2020 21:37
@ashleymichal ashleymichal self-assigned this Mar 2, 2020
@ashleymichal ashleymichal force-pushed the alewis/delay-kv-bulk-delete branch 3 times, most recently from 0b9d7ff to 2f48ff0 Compare March 3, 2020 15:14
@ashleymichal ashleymichal changed the title WIP - Delay KV bulk delete Delay KV bulk delete Mar 3, 2020
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review is mostly me being picky about error messages - this looks great, super happy to get this done

src/commands/kv/bucket/mod.rs Outdated Show resolved Hide resolved
src/commands/publish.rs Show resolved Hide resolved
src/commands/publish.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ashleymichal
Copy link
Contributor Author

update: i'm not necessarily gunning to get this into 1.8.1; i'm happy to wait on 1.9.0. one of the refactors here removes the bucket attribute from kv-namespaces, which from what i could tell was never supported, but i want to do a little extra testing to confirm that there isn't any support here so i don't break unsuspecting crafty users.

@AngusMorton
Copy link

update: i'm not necessarily gunning to get this into 1.8.1; i'm happy to wait on 1.9.0. one of the refactors here removes the bucket attribute from kv-namespaces, which from what i could tell was never supported, but i want to do a little extra testing to confirm that there isn't any support here so i don't break unsuspecting crafty users.

Is there an ETA for 1.9.0 or for this to be released (even as an alpha/RC)?

@ashleymichal ashleymichal changed the title Delay KV bulk delete WIP - Delay KV bulk delete Mar 5, 2020
@ashleymichal ashleymichal mentioned this pull request Mar 5, 2020
4 tasks
@ashleymichal ashleymichal changed the title WIP - Delay KV bulk delete Delay KV bulk delete Mar 11, 2020
@ashleymichal
Copy link
Contributor Author

Is there an ETA for 1.9.0 or for this to be released (even as an alpha/RC)?

We'll be putting this out on its own as a point release (1.8.2)

@ashleymichal ashleymichal modified the milestones: sites clean up, 1.8.2 Mar 11, 2020
@ashleymichal ashleymichal merged commit 3da6ec7 into master Mar 11, 2020
@ashleymichal ashleymichal deleted the alewis/delay-kv-bulk-delete branch March 11, 2020 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Something is broken, but works in previous releases sites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Downtime on Site update
3 participants