-
Notifications
You must be signed in to change notification settings - Fork 593
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
json: minor clean-ups #17403
json: minor clean-ups #17403
Conversation
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
} | ||
w.EndArray(); | ||
} | ||
|
||
template<typename T, size_t chunk_size = 128> | ||
void rjson_serialize( |
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.
Don't we need to fix these other functions too?
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.
By fix I mean move to the public header? The public header is re-exporting these methods right now.
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.
sorry i'm not sure what you mean. what's an example?
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.
Sorry I was confused that the new json.h
file is in container/include/container
, originally I thought you put that in json/include/json
. I am assuming that moving json serialization to the container module is intentional? Should we be doing that for everything? Or maybe we should be doing the serde approach and just have a template version?
I am assuming you're moving rjson_serialize
for fragmented_vector for some cyclical dependency?
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.
Yeh, definitely open to alternatives here. The idea is that it is not scalable for json/json.h
to have overrides for everything (e.g. container/*
net/*
, etc...) and that each module should have its own json.h
for its types. what remains in json/json.h
would be overloads for external dependencies and stuff in base/*.
wdyt?
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.
That SGTM
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46823#018e7c71-c311-4d3b-bd8e-37a1d897fe7e ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46823#018e7d4c-ec99-438f-93da-115d21b5dbea ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46890#018e80af-a1d6-4bca-9421-636cfdc54fd9 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46890#018e80af-a1d4-4184-b182-a04edaf01a3e ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46890#018e80c3-0e53-4b4e-a4a2-41d8ab7a3211 |
new failures in https://buildkite.com/redpanda/redpanda/builds/46890#018e80c3-0e4a-43f1-a78b-6dde60bd926b:
|
Not new #8328 |
json: minor clean-ups
Backports Required
Release Notes