enh(grant): timeseries calculation excluding system tables#34778
enh(grant): timeseries calculation excluding system tables#34778
Conversation
kailixu
requested review from
a team,
dapan1121,
guanshengliang and
hzcheng
as code owners
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the timeseries calculation within the grant system by explicitly excluding a broader set of internal system tables. This change aims to improve the precision of resource usage metrics by focusing on user-defined data. Accompanying this enhancement is the addition of a robust test suite that thoroughly verifies the new exclusion logic and the overall accuracy of timeseries statistics under various DDL operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the timeseries calculation for grants by excluding a list of system tables. The change in vnodeQuery.c adds a significant number of tables to the exclusion list. A new, comprehensive test suite is added in test_grant_basic.py to verify this new logic and other grant-related timeseries calculations.
My review found a few areas for improvement:
- In the C code, the new list of tables should be sorted for better maintainability.
- The new Python test file is missing an import for the
subprocessmodule, which will cause it to fail. - The test file also uses
shell=Trueinsubprocesscalls where it's not necessary, and contains a hardcoded path that could be made more robust.
I've provided suggestions to address these points.
| "taosd_dnodes_status", | ||
| "adapter_conn_pool", | ||
| "taosd_vnodes_info", | ||
| "taosd_dnodes_metrics", | ||
| "taosd_vgroups_info", | ||
| "taos_sql_req", | ||
| "taosd_mnodes_info", | ||
| "adapter_c_interface", | ||
| "taosd_cluster_info", | ||
| "taosd_sql_req", | ||
| "taosd_dnodes_info", | ||
| "adapter_requests", | ||
| "taosd_write_metrics", | ||
| "adapter_status", | ||
| "taos_slow_sql", | ||
| "taos_slow_sql_detail", | ||
| "taosd_cluster_basic", | ||
| "taosd_dnodes_data_dirs", | ||
| "taosd_dnodes_log_dirs", | ||
| "xnode_agent_activities", | ||
| "xnode_task_activities", | ||
| "xnode_task_metrics", | ||
| "taosx_task_csv", | ||
| "taosx_task_progress", | ||
| "taosx_task_kinghist", | ||
| "taosx_task_tdengine2", | ||
| "taosx_task_tdengine3", | ||
| "taosx_task_opc_da", | ||
| "taosx_task_opc_ua", | ||
| "taosx_task_kafka", | ||
| "taosx_task_influxdb", | ||
| "taosx_task_mqtt", | ||
| "taosx_task_avevahistorian", | ||
| "taosx_task_opentsdb", | ||
| "taosx_task_mysql", | ||
| "taosx_task_postgres", | ||
| "taosx_task_oracle", | ||
| "taosx_task_mssql", | ||
| "taosx_task_mongodb", | ||
| "taosx_task_sparkplugb", | ||
| "taosx_task_orc", | ||
| "taosx_task_pulsar", | ||
| "taosx_task_pspace"}; |
There was a problem hiding this comment.
For better readability and maintainability, it's good practice to keep this list of table names sorted alphabetically. This makes it easier to check for existing entries and avoid duplicates in the future.
"adapter_c_interface",
"adapter_conn_pool",
"adapter_requests",
"adapter_status",
"taos_slow_sql",
"taos_slow_sql_detail",
"taos_sql_req",
"taosd_cluster_basic",
"taosd_cluster_info",
"taosd_dnodes_data_dirs",
"taosd_dnodes_info",
"taosd_dnodes_log_dirs",
"taosd_dnodes_metrics",
"taosd_dnodes_status",
"taosd_mnodes_info",
"taosd_sql_req",
"taosd_vgroups_info",
"taosd_vnodes_info",
"taosd_write_metrics",
"taosx_task_avevahistorian",
"taosx_task_csv",
"taosx_task_influxdb",
"taosx_task_kafka",
"taosx_task_kinghist",
"taosx_task_mongodb",
"taosx_task_mqtt",
"taosx_task_mssql",
"taosx_task_mysql",
"taosx_task_opc_da",
"taosx_task_opc_ua",
"taosx_task_opentsdb",
"taosx_task_orc",
"taosx_task_oracle",
"taosx_task_postgres",
"taosx_task_progress",
"taosx_task_pspace",
"taosx_task_pulsar",
"taosx_task_sparkplugb",
"taosx_task_tdengine2",
"taosx_task_tdengine3",
"xnode_agent_activities",
"xnode_task_activities",
"xnode_task_metrics"};| def s3_check_show_grants_granted(self): | ||
| tdLog.printNoPrefix("======== test show grants granted: ") | ||
| try: | ||
| process = subprocess.Popen(f'{self.workPath}{os.sep}grantTest 1', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Using shell=True with subprocess.Popen can be a security risk if the command string is crafted from external input. While the risk is low here, it's a best practice to avoid it when not strictly necessary. You can pass the command and its arguments as a list to Popen instead.
| process = subprocess.Popen(f'{self.workPath}{os.sep}grantTest 1', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | |
| process = subprocess.Popen([f'{self.workPath}{os.sep}grantTest', '1'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
| tdSql.checkRows(3) | ||
|
|
||
| self.genClusterInfo(check=False) | ||
| process = subprocess.Popen(f'{self.workPath}{os.sep}grantTest 2', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Similar to the previous comment, it's better to avoid shell=True. The command can be passed as a list of arguments.
| process = subprocess.Popen(f'{self.workPath}{os.sep}grantTest 2', shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | |
| process = subprocess.Popen([f'{self.workPath}{os.sep}grantTest', '2'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
| - Or the database is created with 'is_audit 1' option | ||
| """ | ||
| # Dynamically parse system supertable arrays from vnodeQuery.c | ||
| vnodeQueryPath = os.path.join(self.TDinternal, "community", "source", "dnode", "vnode", "src", "vnd", "vnodeQuery.c") |
There was a problem hiding this comment.
This hardcoded path to vnodeQuery.c is brittle and might break if the source code directory structure changes. Consider making this more robust. For example, you could determine the repository root programmatically and construct the path from there, or pass the path as a configuration parameter to the test.
1 participant
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.