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

bug: Bring staticnode up-to-date with tokenserver storage #193

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Conversation

jrconlin
Copy link
Member

  • Add node & key_changed_at fields to staticnode db.
  • add **kw to capture other unused args.

Closes #191

@jrconlin jrconlin requested a review from rfk January 14, 2020 21:55
Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

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

add **kw to capture other unused args.

Hrm, I'm not sure about this, it seems like it might risk just masking future errors rather than failing loudly with a signature mismatch, and having them surface only later as incorrect data written into the db. @jrconlin thoughts?

Existins users of this codebase will need to run a db migration to get the new fields, will the existing migration from the tokenserver codebase work correctly here?

@jrconlin
Copy link
Member Author

Hrm, I'm not sure about this, it seems like it might risk just masking future errors rather than failing loudly with a signature mismatch, and having them surface only later as incorrect data written into the db. @jrconlin thoughts?

I view the StaticNode to be kind of a special case. I'll note that we had already added the node and key_changed_at columns and had not checked StaticNode to see it was broken. Being a bit forgiving regarding extra arguments we're going to ignore anyway just means less future breakage.

Existins users of this codebase will need to run a db migration to get the new fields, will the existing migration from the tokenserver codebase work correctly here?

I see that the tokenserver migrations include both of those fields, so I presume that if the migration process works correctly, the table would have already been changed.

Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

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

WFM; thanks for taking this on @jrconlin!

@rfk rfk merged commit 8c2c2ed into master Jan 15, 2020
@jrconlin jrconlin deleted the bug/191 branch January 15, 2020 17:38
@jrconlin
Copy link
Member Author

sigh. looks like the migration script does not fire automatically.
Wohlraj is correct.

I'll see if I can work out what's going on with alembic or at least add instructions to existing syncstorage users.

Mic92 pushed a commit to Mic92/syncserver that referenced this pull request Feb 23, 2022
* debugging code

* Fix get_collection_id and add some debugging cruft

* Fix a few more things

* Fix a few more errors

* Fix some more stuff

* Example of how to convert a timestamp properly and some more bugfixes

* Fix another error

* Fix another timestamp

* wip fill in transaction ids

* checkpoint

* fix nanoseconds/cleanup

* sql_request

* some error handling cleanup
(converting google_spanner1::Error to DbError)

* wip

* wip

* Don't shadow q; don't add a , if we're adding an empty string; manage lifetimes properly in the comma function

* Fix put_bso

* kill

* Parse timestamps as rfc3339, not i64

* Fix get_bso

* Fix get_bso_timestamp

* Switch all timestamp formatting to to_rfc3339 function, fixing bso_successfully_updates_single_values

* Return a map of collection names to counts, not collection ids to counts

* Fix spellling error

* Fix delete_collection

* Fix delete_bsos_in_correct_collection

* ORDER BY not SORT BY

* Fix get_collection_timestamps

* Fix get_collection_usage

* Fix create_collection when the database is empty

* use row_count_exact

* phrasing

* Fix get_bsos_sort

* Make all of the tests pass

* fix up transaction handling:

- delete_storage lacks a collection so there's no lock_for_xxx called
for it
- create_collection may be called within lock_for_write (before the
  transaction is created)

so, like the python version, if no txn is present and one is asked for,
we implicitly create a r/w txn

* Actually fix conflicts

* Switch to eprintln

* followups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StaticNodeAssignment does not match TokenServer
4 participants