Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a proper key_value for the new storage #881

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

Pchelolo
Copy link
Contributor

@Pchelolo Pchelolo commented Oct 3, 2017

  1. Rename key_value to key_value_old and use it everywhere
  2. Adapt the key_value to the new storage - remove unused columns and make a tid a normal column, not a part of a primary key
  3. Create new buckets for the new feeds.

cc @wikimedia/services

Change-Id: If7b60efc92d8aa7c69cce4b6813a4951f91c3718
@d00rman
Copy link
Contributor

d00rman commented Oct 3, 2017

This seems a bit too invasive, IMHO. Wouldn't it be better to use the proxy approach we've been using? Which key_value to use could then be specified in the resources section during bucket creation, something along the lines of:

version: 1
valueType: 'json'
backend: 'new'

with the default being backend: 'old'

@Pchelolo
Copy link
Contributor Author

Pchelolo commented Oct 3, 2017

Then we'd need to specify new/old on very request because we don't really have a space to store whether a particular bucket was created in new/old storage. We can cache that in memory though..

Why do you think it's invasive? it's mostly a mass rename of key_value to key_value_old. We can not do that and name the new bucket key_value_new, but then we'd need to do a mass rename anyways after we switch all to the new bucket.

Change-Id: I10abb802b56e0649a86f4240ec24be4f2eb9da34
@Pchelolo
Copy link
Contributor Author

Pchelolo commented Oct 3, 2017

I've tried switching all the use-cases to the new key_value implementation and the new storage and run the tests and everything looks nice.

const rp = req.params;
let tid = rp.tid && mwUtil.coerceTid(rp.tid, 'key_value');
let tid = rp.tid && mwUtil.coerceTid(rp.tid, 'key_value_old');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/key_value_old/key_value/

@d00rman d00rman merged commit 2275635 into wikimedia:master Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants