-
Notifications
You must be signed in to change notification settings - Fork 697
minimize returned data to avoid large responses #19878
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
Conversation
🟢 |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
96576f6
to
6b2b627
Compare
6b2b627
to
f49227f
Compare
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
f49227f
to
b47687b
Compare
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
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.
Pull Request Overview
This PR introduces selective field rendering to reduce payload sizes by only including data the user explicitly requests, updates handler priorities, and aligns test expectations accordingly.
- Added
FieldsRequested
to mirror user-requested fields and gated JSON output on it - Updated HTTP handler registration order for
/viewer/nodes
and/storage/groups
- Revised canonical JSON tests to remove or reorganize fields that are no longer always returned
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ydb/core/viewer/viewer_nodes.h | Introduce FieldsRequested , gate per-field JSON output |
ydb/core/viewer/storage_groups.h | Same selective-rendering logic for storage groups responses |
ydb/core/viewer/json_handlers_viewer.cpp | Bumped handler priority for viewer nodes endpoint |
ydb/core/viewer/json_handlers_storage.cpp | Bumped handler priority for storage groups endpoint |
ydb/core/viewer/tests/canondata/result.json | Updated expected JSON output to match minimized responses |
Comments suppressed due to low confidence (3)
ydb/core/viewer/viewer_nodes.h:1064
- Document in the Swagger spec (in
TJsonNodes::GetSwagger
) the newfields
query parameter so API consumers know how to request specific fields.
FieldsRequested = FieldsRequired; // no dependent fields
ydb/core/viewer/json_handlers_viewer.cpp:264
- The handler priority was incremented from 15 to 16; verify this change does not inadvertently affect routing order and document the reasoning.
handlers.AddHandler("/viewer/nodes", new TJsonHandler<TJsonNodes>(TJsonNodes::GetSwagger()), 16);
ydb/core/viewer/tests/canondata/result.json:150
- Removed
size_bytes
entries from the canonical tests; consider adding dedicated tests that explicitly request and verifysize_bytes
behavior remains correct when requested.
"owner": "user1",
jsonNode.SetDatabase(node->Database); | ||
} | ||
if (node->UptimeSeconds) { | ||
if (node->UptimeSeconds && FieldsRequested.test(+ENodeFields::Uptime)) { | ||
jsonNode.SetUptimeSeconds(*(node->UptimeSeconds)); | ||
} | ||
if (node->Disconnected) { |
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.
[nitpick] The Disconnected
field is always included; consider gating it with FieldsRequested.test(+ENodeFields::Disconnected)
for consistency with other optional fields.
if (node->Disconnected) { | |
if (node->Disconnected && FieldsRequested.test(+ENodeFields::Disconnected)) { |
Copilot uses AI. Check for mistakes.
TFieldsType FieldsRequested; // fields that were requested by user | ||
TFieldsType FieldsRequired; // fields that are required to calculate the response |
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.
[nitpick] FieldsRequired
and FieldsRequested
names are quite similar and can be confusing; consider renaming to DependentFields
and UserRequestedFields
for clarity.
TFieldsType FieldsRequested; // fields that were requested by user | |
TFieldsType FieldsRequired; // fields that are required to calculate the response | |
TFieldsType UserRequestedFields; // fields that were requested by user | |
TFieldsType DependentFields; // fields that are required to calculate the response |
Copilot uses AI. Check for mistakes.
"MaxTableColumns": "200", | ||
"MaxTableIndices": "20", | ||
"MaxTableKeyColumns": "30" | ||
}, | ||
"SchemeShardId_Depricated": "72057594046678944", |
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.
The field SchemeShardId_Depricated
appears to be misspelled; consider renaming to SchemeShardId_Deprecated
to match conventional spelling.
"SchemeShardId_Depricated": "72057594046678944", | |
"SchemeShardId_Deprecated": "72057594046678944", |
Copilot uses AI. Check for mistakes.
Changelog entry
return requested data only, minimizes huge responses, especially on network pages, closes #19810
Changelog category
Description for reviewers
...