-
Notifications
You must be signed in to change notification settings - Fork 38
RHINENG-16453: gather table sizes from all used schemas, not only public #1883
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
RHINENG-16453: gather table sizes from all used schemas, not only public #1883
Conversation
Referenced Jiras: |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR enhances table size gathering by fully qualifying table names across multiple schemas, updates corresponding tests for the new key format, and refines the README and test script to simplify running individual tests in containers or locally. ER diagram for table size gathering across multiple schemaserDiagram
PG_TABLES {
string schemaname
string tablename
}
TABLE_SIZES {
string key
int value
}
PG_TABLES ||--o| TABLE_SIZES : "generates size for"
Class diagram for updated getTableSizes function and keyValue usageclassDiagram
class keyValue {
+string key
+int value
}
class metrics_db {
+getTableSizes() []keyValue
}
metrics_db --> keyValue: returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider dynamically determining which schemas to include (e.g. exclude system schemas) instead of hardcoding public, inventory, and repack.
- Use quote_ident on both schemaname and tablename when building the identifier in pg_total_relation_size to avoid any edge cases with special characters.
- Add an assertion in TestTableSizes to verify that tables from the repack schema are included, similar to the inventory.hosts_v1_0 check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider dynamically determining which schemas to include (e.g. exclude system schemas) instead of hardcoding public, inventory, and repack.
- Use quote_ident on both schemaname and tablename when building the identifier in pg_total_relation_size to avoid any edge cases with special characters.
- Add an assertion in TestTableSizes to verify that tables from the repack schema are included, similar to the inventory.hosts_v1_0 check.
## Individual Comments
### Comment 1
<location> `README.md:68` </location>
<code_context>
+- Run the same command as for running all tests (from above)
+
+### Run single test locally
After running all test suit, testing platform components are still running (kafka, platform, db). This is especially useful when fixing some test or adding a new one. You need to have golang installed.
~~~bash
podman-compose -f docker-compose.test.yml up --build --no-start # build images
</code_context>
<issue_to_address>
**issue (typo):** Typo: 'test suit' should be 'test suite'.
Please update the wording to 'test suite'.
```suggestion
After running all test suite, testing platform components are still running (kafka, platform, db). This is especially useful when fixing some test or adding a new one. You need to have golang installed.
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Can one of the admins verify this patch? |
e49c695
to
5121e93
Compare
/ok-to-test |
5121e93
to
e7745c5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1883 +/- ##
=======================================
Coverage 57.58% 57.58%
=======================================
Files 131 131
Lines 10197 10198 +1
=======================================
+ Hits 5872 5873 +1
Misses 3791 3791
Partials 534 534
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/ok-to-test |
/retest |
420a34a
to
33973ec
Compare
Description
This PR:
getTableSizes
function to gather data about tables from all used schemas, not just publicSecure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Fix table size collection to include all schemas, update tests to reflect schema-qualified table names, and improve test-run documentation.
Bug Fixes:
Documentation:
Tests: