-
Notifications
You must be signed in to change notification settings - Fork 123
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
universe: sync "missing universe id" when syncing unknown asset ID #1316
Conversation
This commit shows what's wrong/weird with the response of the QueryAssetRoots RPC when a root doesn't exist. We can't change this behavior right away as that would potentially break older sync clients, if we just started returning an error. So we'll first update the syncer to _expect_ a universe.ErrNoUniverseRoot error, then in a future version we can start returning that.
This is the actual preparation for being able to return an error in the future. We start handling the error as non-terminating in the syncer.
This prevents the actual error with current versions of the universe by detecting if a response is empty (and should've just been the ErrNoUniverseRoot which future servers will respond).
Pull Request Test Coverage Report for Build 12915294932Details
💛 - Coveralls |
How does this change affect older nodes? If an older node attempts to sync an asset ID that is unknown to a universe server implementing this change, the older node will receive an error. Does this terminate the universe syncing process for that server? Maybe we should add a version field to RPC endpoint's |
There are no errors returned yet. I tried to make that very clear in the commit messages and all the comments. We're preparing for being able to deal with an error being returned but we don't return it yet. We'll return errors with #1315, which will be done in 0.6 or 0.7. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we start returning this new error, syncing might break for v0.5.0 nodes and older. But we're ok with that because the new group key formulation is only understood by nodes v0.5.1 and newer anyway.
So we expect everyone to upgrade their nodes past v0.5.0 soon enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm 🚢
Fixes #990, at least partially.
We'll need another fix in future versions and actually return an error properly (#1315).
But to not break older clients, we'll only do that after a full version (e.g. we ship this fix in 0.5.1, then with 0.6 or 0.7 we return an actual error, knowing that enough clients will understand to not interrupt the whole sync process).