Skip to content

feat: better Serializer for MutableConfigValue - #8421

Merged
ppca merged 3 commits into
near:masterfrom
ppca:xiangyiz/ND-283
Feb 1, 2023
Merged

feat: better Serializer for MutableConfigValue#8421
ppca merged 3 commits into
near:masterfrom
ppca:xiangyiz/ND-283

Conversation

@ppca

@ppca ppca commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

When we enable dynamically config, we use a MutableConfigValue type to represent the mutable fields.
Only value field is valuable to end users. This PR aims to only show MutableConfigValue.value to users and hide the other 2 fields.
For example, if you navigate to /debug/client_config, and look at the field expected_shutdown, this PR should show
"expected_shutdown": "null"
instead of
"expected_shutdown: "expected_shutdown":{"value":null,"field_name":"expected_shutdown","last_update":"2023-01-23T19:26:43.817152Z"}

@ppca
ppca marked this pull request as ready for review January 23, 2023 19:29
@ppca
ppca requested a review from a team as a code owner January 23, 2023 19:29
@ppca
ppca requested review from jakmeier and nikurt and removed request for jakmeier January 23, 2023 19:29
@ppca

ppca commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

prev discussion about why we need a custom serializer implementation: #8400

@nikurt

nikurt commented Jan 27, 2023

Copy link
Copy Markdown
Contributor

Please update the title according to https://github.com/near/nearcore/blob/master/CONTRIBUTING.md#pull-requests

For example: "feat: Better Serializer for MutableConfigValue"

/// See `expected_shutdown` for an example how to use it.
/// TODO: custom implementation for Serialize and Deserialize s.t. only value is necessary(JIRA:ND-283)
#[derive(Clone, Debug, Serialize, Deserialize)]
#[derive(Clone, Debug, Deserialize)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The derived implementation of Deserialize is not compatible with the custom Serialize implementation. Please consider removing the derived Deserialize implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the derived deserializer for RpcClientConfigResponse, ClientConfig and MutableConfigValue. The /client_config is still working perfectly fine locally. Tested and confirmed serializer is needed here.
Also removed derived Debug for RpcClientConfigRequest and RpcClientConfigResponse as Debug does not seem necessary.

@ppca ppca changed the title Serializer for MutableConfigValue feat: better serializer for MutableConfigValue Jan 30, 2023
@ppca ppca changed the title feat: better serializer for MutableConfigValue feat: better Serializer for MutableConfigValue Jan 30, 2023
@ppca
ppca merged commit c8b729c into near:master Feb 1, 2023
@ppca
ppca deleted the xiangyiz/ND-283 branch February 1, 2023 15:16
nikurt pushed a commit that referenced this pull request Feb 6, 2023
* better Serializer for MutableConfigValue: only display value to users

* remove derived Deserialize and DEBUG for RpcClientConfigResponse, ClientConfig, MutableConfigValue

* remove RpcClientConfigRequest
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.

2 participants