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

Serialize page token with MessagePack instead of as JSON string #924

Closed
wants to merge 1 commit into from

Conversation

david-crespo
Copy link
Contributor

Proof of concept showing how easy this is to change. Would close #923 if we like it. Considerations:

  • We should get a better sense than I did in Make page tokens shorter #923 of how much of an improvement this is in the real world
  • Should we try compression also/instead?
  • Do we need to rev the PaginationVersion? Currently, Nexus deploys involve shutting down all nodes, so the problem of old and new formats being used at the same time isn't likely to be a big problem, plus we'd have to add logic for handling both formats
    /// Version for the pagination token serialization format
    ///
    /// This may seem like overkill, but it allows us to rev this in a future version
    /// of Dropshot without breaking any ongoing scans when the change is deployed.
    /// If we rev this, we might need to provide a way for clients to request at
    /// runtime which version of token to generate so that if they do a rolling
    /// upgrade of multiple instances, they can configure the instances to generate
    /// v1 tokens until the rollout is complete, then switch on the new token
    /// version. Obviously, it would be better to avoid revving this version if
    /// possible!
    ///
    /// Note that consumers still need to consider compatibility if they change their
    /// own `ScanParams` or `PageSelector` types.
    #[derive(Copy, Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
    #[serde(rename_all = "lowercase")]
    enum PaginationVersion {
    V1,
    }

@david-crespo
Copy link
Contributor Author

This seemed fine, but @ahl talked me out of putting these cursors in the query string in console, so will close for now.

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.

Make page tokens shorter
1 participant