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

Bump protocol version to 3 #173

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Bump protocol version to 3 #173

merged 2 commits into from
Jan 3, 2024

Conversation

lawrence-forooghian
Copy link
Collaborator

In order to get the flattened stats API (this change should have come in a731d12).

@lawrence-forooghian
Copy link
Collaborator Author

@SimonWoolf you mentioned on Slack that "you can request flat stats even with library protocol v2, if there's some particular reason you don't want to upgrade the lib protocol version yet, by specifying flatStats=true as a request param". I'm assuming that we'd prefer that client libraries bump the protocol version as I've done here, though?

owenpearson
owenpearson previously approved these changes Dec 1, 2023
@SimonWoolf
Copy link
Member

if there's some particular reason you don't want to upgrade the lib protocol version yet, by specifying flatStats=true as a request param". I'm assuming that we'd prefer that client libraries bump the protocol version as I've done here, though?

sure -- only thing to bear in mind is that v=3 also triggers the new batched response, which means it's a breaking api change for anyone using rest.request() to do batched responses, which means that change should probably be made only in a library new major version, per semver. (But you'd probably want to do that anyway because completely changing the type of the rest.stats() response is also a breaking api change)

@lawrence-forooghian
Copy link
Collaborator Author

lawrence-forooghian commented Dec 1, 2023

v=3 also triggers the new batched response

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

which means it's a breaking api change for anyone using rest.request() to do batched responses

I think this is no longer an issue now that Rest.request() requires the user to provide a version parameter (RSC19f1)? (I mean, that is itself a breaking change which is going into the same spec version as this PR targets, so it's kinda moot)

@lawrence-forooghian
Copy link
Collaborator Author

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

(I'll need to merge main into integration/3.0.0 to pull in the batch stuff first before I can do this, if one of you could approve #174 that would be great)

@SimonWoolf
Copy link
Member

That probably means that alongside this protocol bump I should update the batch-related spec points to remove the requirement that they pass the newBatchResponse=true query param, right?

sure go for it

I think this is no longer an issue now that Rest.request() requires the user to provide a version parameter (RSC19f1)? (I mean, that is itself a breaking change which is going into the same spec version as this PR targets, so it's kinda moot)

yeah exactly, it fixes this problem for future breaking changes, but can't really do it retrospectively

(I'll need to merge main into integration/3.0.0 to pull in the batch stuff first before I can do this, if one of you could approve #174 that would be great)

I've changed the protection rules, you shouldn't need a pr review for a routine rebase of an integration branch, you can now self-review

@lawrence-forooghian
Copy link
Collaborator Author

I've pushed a new commit for the batch changes, mind re-reviewing please @owenpearson?

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

In order to get the flattened stats API (this change should have come in
a731d12).
Protocol version 3 (which we started using in 943be9c) gives the new batch
response by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants