[Bugfix] [Platform] Increase memory limit for Inventory #1973
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a bugfix to address memory issues with the Inventory feature by changing how inventory data is served from an in-memory static response to a file-based approach.
- Adds a new
ConfigDestinationTypeFiledestination type for serving files directly - Refactors inventory handling to write data to a file and serve it via file path instead of keeping it in memory
- Updates checksum calculation to include inventory data for proper change detection
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/constants/gateway.go | Adds constant for inventory filename |
| pkg/deployment/resources/gateway/gateway_config_destination_type.go | Introduces new file destination type and updates validation logic |
| pkg/deployment/resources/gateway/gateway_config_destination_static.go | Defines file destination interface and implementation |
| pkg/deployment/resources/gateway/gateway_config_destination.go | Implements file destination validation and route action handling |
| pkg/deployment/resources/config_map_gateway.go | Refactors inventory from static response to file-based serving with updated checksum logic |
| CHANGELOG.md | Documents the bugfix for inventory memory limit increase |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| if c.Type.Get() == ConfigDestinationTypeFile { | ||
| if c.File == nil { | ||
| return errors.Errorf("File response is not defined!") |
There was a problem hiding this comment.
The error message uses informal language with an exclamation mark. Consider using a more formal message like 'File response configuration is required' for consistency with other error messages in the codebase.
| return errors.Errorf("File response is not defined!") | |
| return errors.Errorf("File response configuration is required") |
| return errors.WithStack(errors.Wrapf(err, "Failed to render gateway inventory")) | ||
| } | ||
|
|
||
| gatewayChecksum := util.SHA256FromStringArray(gatewayCfgYamlChecksum, util.SHA256(inventoryData)) |
There was a problem hiding this comment.
The checksum calculation combines two different hash inputs (string and byte array hash). Consider using a consistent approach by either converting both to strings or both to byte arrays for clarity.
| gatewayChecksum := util.SHA256FromStringArray(gatewayCfgYamlChecksum, util.SHA256(inventoryData)) | |
| gatewayChecksum := util.SHA256FromStringArray(gatewayCfgYamlChecksum, fmt.Sprintf("%x", util.SHA256(inventoryData))) |
ajanikow
force-pushed
the
bugfix/increase_memory_limit_for_inv
branch
from
e8829aa to
e8ed542
Compare
No description provided.