cp to 1.1: fix create account timeout #14182
Conversation
原因： 传给cn，tn的context，某些情况下没有带上accountid。原先没带上accountid，默认就是sys租户。 mo后台任务线程默认就是sys租户。创建普通租户时，普通租户会创建一些表。如果此时accountid没设置， 就会被当作sys租户，来创建一些表。会与后台线程冲突。如果后台线程执行时间长，表现为创建租户时卡住。 修改： 在context没有设置accountid时，立即报错。避免进一步出错。 后续根据报错，修改代码，传入accountid。 影响： 修改了所有从context取accountid的地方。改动较多位置。但是改动都很直观。 Approved by: @m-schen, @aptend, @aunjgr, @ouyuanning, @reusee, @qingxinhome, @nnsgmsone, @triump2020, @jiangxinmeng1, @XuPeng-SH, @zhangxu19830126, @iamlinjunhong, @badboynt1, @sukki37
daviszhen
requested review from
reusee,
XuPeng-SH,
nnsgmsone,
triump2020,
aptend,
zhangxu19830126,
m-schen,
ouyuanning,
aunjgr,
badboynt1,
iamlinjunhong,
qingxinhome and
sukki37
as code owners
|
@daviszhen Thanks for your contributions! Here are review comments for file pkg/bootstrap/bootstrap.go: Body:
Changes in pkg/bootstrap/bootstrap.go:
Review:
Suggestions:
Here are review comments for file pkg/cnservice/upgrader/upgrader.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/cnservice/upgrader/upgrader.go:
Overall, the changes made in this pull request address the issue of creating an account timeout by ensuring that the system account is always attached to the context. The changes seem reasonable and straightforward. However, there are a few suggestions for optimization and security improvements:
Other than these suggestions, the changes in the pull request seem appropriate and should address the issue of create account timeout. Here are review comments for file pkg/cnservice/upgrader/upgrader_util.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/cnservice/upgrader/upgrader_util.go:
Suggestions for improvement:
Here are review comments for file pkg/defines/type.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/defines/type.go:
Overall, the changes made in this pull request address the issue of the account ID not being set in the context and provide a way to retrieve and attach the account ID, user ID, and role ID to the context. The changes seem to be necessary and straightforward. Suggestions for improvement:
Here are review comments for file pkg/frontend/cmd_executor.go: Body:
Changes in pkg/frontend/cmd_executor.go:
Potential Problems:
Suggestions:
Optimizations: Here are review comments for file pkg/frontend/compiler_context.go: Body: This pull request fixes an issue where the context passed to cn and tn sometimes does not include the account ID. Previously, when the account ID was not included, the default was set to the sys tenant. This caused conflicts with the background task thread, which is also set to the sys tenant. When creating a regular tenant, the tenant would create some tables. If the account ID was not set, the tables would be created as sys tenant tables, causing conflicts with the background thread. This resulted in a timeout when creating the tenant. To fix this issue, an error is now thrown immediately when the account ID is not set in the context. This prevents further errors. The code is then modified to pass the account ID based on the error thrown. The changes in this pull request modify the Overall, these changes affect multiple places where the account ID is retrieved from the context. The modifications are straightforward and intuitive. Review:
Optimization suggestions:
Security concerns: No security concerns were identified in this pull request. Here are review comments for file pkg/frontend/computation_wrapper.go: Title: cp to 1.1: fix create account timeout Body:
Changes in computation_wrapper.go:
Overall, the pull request addresses the issue of the create account timeout by fixing the handling of the account ID in the context. The changes made in computation_wrapper.go appear to be correct and should resolve the problem. No security issues were identified in the pull request. Here are review comments for file pkg/frontend/mysql_cmd_executor_test.go: Body: This pull request fixes an issue where the context passed to To fix this issue, an error is immediately thrown when the Changes in
Overall, the changes made in this pull request address the issue of the missing However, there are a few areas that could be improved:
Overall, the changes in this pull request effectively address the issue and provide a solution to avoid conflicts and timeouts during the creation of a tenant. The suggested improvements will help enhance the code quality and maintainability of the affected functions and test cases. Here are review comments for file pkg/frontend/plan_cache.go: Title: cp to 1.1: fix create account timeout Body:
Changes in
Suggestions:
Security Issues:
Here are review comments for file pkg/frontend/routine.go: Body:
Changes in routine.go:
Review:
Suggestions:
Here are review comments for file pkg/frontend/session.go: Title: cp to 1.1: fix create account timeout Body: The pull request fixes a timeout issue when creating an account. The problem occurs when the context passed to cn and tn does not include the account ID. By default, if the account ID is not set, it is assumed to be the sys tenant. This causes conflicts with the background task thread, especially when creating a normal tenant that creates some tables. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. This change affects all places where the account ID is retrieved from the context, but the changes are straightforward. Changes in session.go:
Optimizations:
Security Issues: Here are review comments for file pkg/frontend/session_test.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/frontend/session_test.go:
Overall, this pull request addresses a bug related to the creation of accounts timing out. The fix involves modifying the code to include the account ID in the context. The changes made in the Suggestions for improvement:
Here are review comments for file pkg/frontend/show_account.go: Body: This pull request fixes an issue where the account ID was not being passed to the Changes in
Overall, this pull request addresses the issue of the account ID not being passed correctly in certain contexts, which was causing conflicts and timeouts when creating a new account. Potential problems and suggestions:
Overall, this pull request addresses the immediate issue and makes the necessary changes to fix it. However, it's important to consider the security implications and optimize the code further if possible. Additionally, improving the error handling and providing clear error messages will enhance the usability of the code. Here are review comments for file pkg/frontend/txn.go: Body: This pull request fixes an issue where the account ID was not being passed to the context when creating a transaction. This caused conflicts with the background task thread and could result in a timeout when creating a tenant. The fix adds an error check and immediately returns an error if the account ID is not set in the context. It also modifies the code to pass the account ID to the context in subsequent code changes. Changes in pkg/frontend/txn.go:
Review:
Overall, the changes in this pull request address the issue described, but there are some areas that need improvement. Here are review comments for file pkg/frontend/util_test.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused conflicts with the background task thread, resulting in a timeout when creating a new account. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID accordingly. This change affects multiple places where the account ID is retrieved from the context. Changes in pkg/frontend/util_test.go:
Review:
Overall, the changes made in this pull request address the issue of the account ID not being passed to the contexts correctly. However, there are opportunities for improvement in terms of error handling and documentation. Here are review comments for file pkg/incrservice/allocator.go: Title: cp to 1.1: fix create account timeout Body:
Changes in
Overall, the changes made in this pull request seem to address the issue of the accountid not being set in the context in certain cases. The use of
Overall, the changes in this pull request address the issue at hand and improve the reliability of the code. However, there are some areas that can be further optimized and improved to enhance security, error handling, code organization, and maintainability. Here are review comments for file pkg/incrservice/allocator_test.go: Title: cp to 1.1: fix create account timeout Body:
Changes in
Overall, the pull request addresses a specific issue related to the create account timeout and makes the necessary changes to fix it. The changes in Suggestions:
Here are review comments for file pkg/incrservice/column_cache.go: Body: This pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. This causes a conflict with the background task thread, which defaults to the sys tenant. When creating a regular tenant, tables are created for that tenant. If the account ID is not set, it is treated as the sys tenant and tables are created, resulting in a conflict with the background thread. This can cause the creation of a tenant to hang if the background thread takes a long time to execute. To fix this issue, an error is immediately thrown when the account ID is not set in the context. The code is then modified to pass in the account ID based on the error. This change affects all places where the account ID is retrieved from the context, but the modifications are straightforward. Changes in pkg/incrservice/column_cache.go:
Review:
Suggestions:
Security Issues:
Here are review comments for file pkg/incrservice/column_cache_test.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused problems when creating a new tenant, as the default account ID was set to sys tenant. This conflicted with the background task thread, which also defaulted to sys tenant. As a result, when creating a tenant, the process would hang if the background thread took a long time to execute. To fix this issue, an error is now thrown immediately if the account ID is not set in the context. This prevents further errors from occurring. The code has been modified to pass the account ID based on the error thrown. The changes made in this pull request affect multiple locations where the account ID is retrieved from the context. However, the changes are straightforward and intuitive. Changes in file pkg/incrservice/column_cache_test.go:
Review:
Suggestions for improvement:
Overall, the changes seem necessary to fix the issue and the code modifications are straightforward. However, providing more context and test cases would improve the quality of the pull request. Here are review comments for file pkg/incrservice/service.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/incrservice/service.go:
Overall, the changes made in this pull request address the issue of not including the account ID in the context in certain cases. The changes ensure that an error is thrown if the account ID is not set, and the code is modified to pass in the account ID where necessary. The changes also include error handling. However, there are a few areas that could be improved:
Suggestion: Add error handling in the
Suggestion: Consider the impact of changing the logging level and ensure that it is necessary for the specific use case.
Suggestion: Move the Overall, the changes made in this pull request address the issue and include error handling. However, there are some areas that could be improved for better code organization and error handling. Here are review comments for file pkg/incrservice/service_test.go: Body: This pull request fixes an issue where the context passed to cn and tn sometimes does not include the account ID. This causes problems when creating a new tenant, as the default account ID is set to "sys" tenant. The background task thread is also set to the "sys" tenant. When creating a new tenant without setting the account ID, it creates tables as the "sys" tenant, which conflicts with the background thread. This can result in a timeout when creating a tenant. To fix this issue, the code now immediately throws an error when the account ID is not set in the context. This prevents further errors. The code will be modified later to pass in the account ID based on the error. This change affects all places where the account ID is retrieved from the context. The changes are straightforward and intuitive. Changes in file pkg/incrservice/service_test.go:
Review:
Suggestions:
Here are review comments for file pkg/incrservice/store_sql.go: To address this issue, the pull request proposes the following changes in @@ -122,9 +122,13 @@ func (s *sqlStore) Allocate(
res.Close()
if rows != 1 {
+ accountId, err := defines.GetAccountId(ctx)
+ if err != nil {
+ return err
+ }
getLogger().Fatal("BUG: read incr record invalid",
zap.String("fetch-sql", fetchSQL),
- zap.Any("account", ctx.Value(defines.TenantIDKey{})),
+ zap.Any("account", accountId),
zap.Uint64("table", tableID),
zap.String("col", colName),
zap.Int("rows", rows),
@@ -149,9 +153,13 @@ func (s *sqlStore) Allocate(
if res.AffectedRows == 1 {
ok = true
} else {
+ accountId, err := defines.GetAccountId(ctx)
+ if err != nil {
+ return err
+ }
getLogger().Fatal("BUG: update incr record returns invalid affected rows",
zap.String("update-sql", sql),
- zap.Any("account", ctx.Value(defines.TenantIDKey{})),
+ zap.Any("account", accountId),
zap.Uint64("table", tableID),
zap.String("col", colName),
zap.Uint64("affected-rows", res.AffectedRows),The changes involve adding code to retrieve the Overall, the changes seem reasonable and address the timeout issue by ensuring that the correct
By addressing these suggestions, the code will be more robust, maintainable, and efficient. Here are review comments for file pkg/incrservice/types.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused problems when creating a new tenant, as the default tenant was being used instead. This resulted in conflicts with the background task thread and could cause the creation of the tenant to hang. The fix is to immediately throw an error when the account ID is not set in the context, and then modify the code to pass in the account ID based on the error. This change affects all places where the account ID is retrieved from the context, but the changes are straightforward. Changes in pkg/incrservice/types.go:
Review:
Suggestions:
Here are review comments for file pkg/sql/compile/compile.go: Title: cp to 1.1: fix create account timeout Body:
Changes in compile.go:
Security Issues:
Suggestions for Optimization:
Here are review comments for file pkg/sql/compile/compile_test.go: Body: This pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. This can cause conflicts with the background task thread, resulting in a timeout when creating a tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass in the account ID based on the error. This change affects all places where the account ID is retrieved from the context, but the changes are straightforward. Changes in
Review:
Suggestions:
Here are review comments for file pkg/sql/compile/ddl.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn context in certain cases. This caused a conflict with the background task thread, resulting in a timeout when creating a tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. This change affects multiple places where the account ID is retrieved from the context, but the changes are straightforward. Changes in pkg/sql/compile/ddl.go:
Review:
Suggestions:
Here are review comments for file pkg/sql/compile/scope.go: Title: cp to 1.1: fix create account timeout Body:
Changes in scope.go:
Security Issues: Suggestions for Optimization: Here are review comments for file pkg/sql/compile/scopeRemoteRun.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/sql/compile/scopeRemoteRun.go:
Overall, this pull request addresses a bug related to the creation of accounts timing out. The fix ensures that the account ID is properly set in the context, avoiding conflicts with the background thread. The changes made in the pull request are straightforward and directly address the issue. Suggestions for improvement:
Here are review comments for file pkg/sql/compile/scopeRemoteRunTypes.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/sql/compile/scopeRemoteRunTypes.go:
Overall, the pull request addresses a bug related to the creation of accounts timing out. The fix involves ensuring that the account ID is properly set in the context to avoid conflicts with the background thread. The changes made in the code seem appropriate and straightforward. However, there are a few suggestions for improvement:
In terms of security, there are no apparent security issues introduced by these changes. Here are review comments for file pkg/sql/compile/scopeRemoteRun_test.go: Body: This pull request fixes an issue where the account ID was not being passed to the context when calling certain functions. This caused conflicts with background threads and resulted in a timeout when creating a new account. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID accordingly. This change affects multiple places where the account ID is retrieved from the context. Changes in file pkg/sql/compile/scopeRemoteRun_test.go:
Review:
Suggestions:
Overall, the pull request seems to address a specific issue related to creating an account timeout. The proposed solution appears to be reasonable, but it would be beneficial to provide more context and include test cases to ensure the fix is effective. Additionally, it's important to review the code changes for any potential security vulnerabilities and consider optimizing the affected code for better performance. Here are review comments for file pkg/sql/compile/scope_test.go: Title: cp to 1.1: fix create account timeout Body:
Changes in scope_test.go:
Suggestions:
Here are review comments for file pkg/sql/compile/sql_executor_context.go: Title: cp to 1.1: fix create account timeout Body:
Changes in
Review Summary:
Here are review comments for file pkg/sql/parsers/tree/expr.go: Title: cp to 1.1: fix create account timeout The title of the pull request is clear and concise, indicating that the purpose of the pull request is to fix a timeout issue related to creating an account. Body: The body of the pull request provides some additional context and information about the changes made. It mentions that the issue being fixed is #13944. It also explains the reason for the issue, which is that in certain cases, the context passed to cn and tn does not include the account ID. This results in the default account being used, which can cause conflicts with background tasks. The proposed solution is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. Changes: The changes made in the pull request are focused on the file Suggestions:
Overall, the pull request addresses a specific issue related to create account timeout. However, there are some areas that need clarification and further explanation. Additionally, it is important to ensure that the changes do not introduce any security vulnerabilities and that the impact of the changes is thoroughly evaluated. Here are review comments for file pkg/sql/plan/build_alter_table.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused a conflict with the background task thread, resulting in a timeout when creating a tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. This change affects all places where the account ID is retrieved from the context. Changes made in
Overall, this pull request addresses a bug related to the account ID not being set correctly in certain contexts, which caused conflicts and timeouts when creating a tenant. The changes made ensure that the account ID is properly passed and checked in the affected functions. However, there are a few issues and suggestions for improvement in this pull request:
To optimize the changes made in this pull request, the following suggestions can be considered:
Overall, this pull request addresses a critical bug related to the account ID not being set correctly in certain contexts. The suggested improvements and optimizations can help enhance the code quality, maintainability, and security of the system. Here are review comments for file pkg/sql/plan/build_constraint_util.go: Title: cp to 1.1: fix create account timeout Body:
Changes in build_constraint_util.go:
Suggestions:
Here are review comments for file pkg/sql/plan/build_ddl.go: Title: cp to 1.1: fix create account timeout Body: The pull request fixes an issue where the account ID is not being passed to the Changes in
Overall, the changes made in this pull request address the issue of not passing the account ID to the
To address these issues, the following suggestions can be implemented:
By addressing these problems and implementing the suggested improvements, the codebase will be more robust and maintainable. Here are review comments for file pkg/sql/plan/build_ddl_test.go: Body: This pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. Without the account ID, the default is set to the sys tenant. This causes conflicts with the background task thread, which is also set to the sys tenant. When creating a regular tenant, some tables are created. If the account ID is not set, these tables will be created as sys tenant tables, causing conflicts with the background thread. This can result in the creation of the tenant getting stuck. To fix this issue, an error is immediately thrown when the account ID is not set in the context. This prevents further errors. The code is then modified to pass the account ID based on the error thrown. The changes in this pull request modify all the places where the account ID is retrieved from the context. The changes are significant but straightforward. Changes in file pkg/sql/plan/build_ddl_test.go:
Review:
Suggestions:
Overall, the pull request addresses the timeout issue related to creating an account and provides a clear explanation of the changes made. However, there is room for improvement in terms of providing more specific impact information and optimizing the changes. Additionally, it is important to ensure proper error handling and security considerations. Here are review comments for file pkg/sql/plan/build_show.go: Title: cp to 1.1: fix create account timeout Body: The pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. This can cause conflicts with the background task thread and result in a timeout when creating a tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. The changes affect multiple places where the account ID is retrieved from the context, but the changes are straightforward. Changes in pkg/sql/plan/build_show.go:
Overall, the changes made in the pull request seem to address the issue of not including the account ID in the context properly. The addition of error handling when retrieving the account ID ensures that any errors are caught and returned, preventing potential issues with creating tenants. The changes appear to be straightforward and do not introduce any new security issues. To optimize the changes, it would be helpful to refactor the code to reduce duplication. Instead of checking for an error and returning it in each function, a helper function could be created to handle the retrieval of the account ID and return any errors. This would make the code more concise and easier to maintain. Additionally, it would be beneficial to add unit tests to verify the correctness of the changes and ensure that they do not introduce any regressions. Here are review comments for file pkg/sql/plan/function/func_mo.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused conflicts with the background task thread, resulting in a timeout when creating a new account. The fix ensures that the account ID is set in the context and avoids further errors. The changes affect multiple places where the account ID is retrieved from the context. Changes in pkg/sql/plan/function/func_mo.go:
Overall, the changes seem to address the issue described in the pull request. However, there are a few areas that could be improved:
To optimize the changes, I would suggest the following:
Overall, the changes seem to address the issue effectively, but the suggested optimizations will help improve the code quality and maintainability. Here are review comments for file pkg/sql/plan/mock.go: Body: This pull request fixes an issue (#13944) where the context passed to cn and tn does not include the account ID in certain cases. Without the account ID, the default is set to the sys tenant. This causes conflicts with the background task thread, which is also set to the sys tenant. When creating a regular tenant, some tables are created. If the account ID is not set, these tables will be created as sys tenant tables, leading to conflicts with the background thread. This can result in a timeout when creating a tenant. To fix this issue, an error is immediately thrown when the account ID is not set in the context. This prevents further errors. The code is then modified to pass the account ID based on the error thrown. The changes made in the pull request are in the file Review:
Suggestions:
Overall, the pull request addresses the specific issue of creating an account timeout and provides a clear solution. The changes made are minimal and straightforward. However, providing more information and context would improve the clarity and completeness of the pull request. Here are review comments for file pkg/sql/plan/query_builder.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/sql/plan/query_builder.go:
Potential Problems:
Suggestions for Improvement:
Here are review comments for file pkg/sql/plan/types.go: Title: cp to 1.1: fix create account timeout The title of the pull request is clear and concise, indicating that the purpose of the changes is to fix a timeout issue related to creating an account. Body: The body of the pull request provides some additional context and information about the changes. It mentions that the issue being fixed is #13944. It also explains the reason for the issue, which is that in certain cases, the context being passed to cn and tn does not include the account ID. This results in the default account ID being used, which can cause conflicts with background tasks. The proposed solution is to immediately throw an error when the account ID is not set in the context, and then modify the code to pass in the account ID based on the error. Changes: The changes made in the pull request are focused on the file Suggestions:
Optimizations: Based on the provided information, it is difficult to suggest specific optimizations for the changes made in this pull request. However, it is always a good practice to review the code for any potential performance improvements or code simplifications. Additionally, it would be helpful to ensure that the changes made in this pull request do not introduce any unnecessary complexity or overhead. Here are review comments for file pkg/sql/plan/types_mock.go: Body: This pull request fixes an issue where the account ID was not being passed to the cn and tn contexts in certain cases. This caused conflicts with the background task thread, resulting in a timeout when creating a tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID based on the error. This change affects multiple places where the account ID is retrieved from the context. Changes in file pkg/sql/plan/types_mock.go:
Here are review comments for file pkg/testutil/testengine/testengine.go: Body: This pull request fixes an issue where the account ID was not being passed to the context when calling certain functions. This caused conflicts with background threads and resulted in timeouts when creating a new account. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass the account ID accordingly. This change affects multiple places where the account ID is retrieved from the context, but the changes are straightforward. Changes made in pkg/testutil/testengine/testengine.go:
Review:
Suggestion: Implement proper authorization and validation checks before attaching the account ID to the context. This can include verifying the user's identity, checking permissions, and ensuring that the account ID is valid and belongs to the user.
Suggestion: Instead of modifying the context in every function call, consider modifying it only when the account ID is not already set. This can be done by checking if the account ID is already present in the context before attaching the system account ID.
Suggestion: Add comments to explain the purpose of attaching the account ID to the context and the reason for the changes made. This will make it easier for other developers to review and understand the code. Overall, the changes made in this pull request address a specific issue related to the account ID not being passed correctly. However, there are security concerns and opportunities for optimization and code quality improvements that should be addressed before merging the changes. Here are review comments for file pkg/util/export/merge.go: Title: cp to 1.1: fix create account timeout Body:
Changes in merge.go:
Suggestions for improvement:
Here are review comments for file pkg/util/metric/mometric/cron_task.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/util/metric/mometric/cron_task.go:
Review:
Suggestions for improvement:
Here are review comments for file pkg/util/metric/mometric/metric.go: Body: This pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. Without the account ID, the default is set to the sys tenant. This causes conflicts with the background task thread, which is also set to the sys tenant. When creating a regular tenant, the regular tenant creates some tables. If the account ID is not set at this time, it will be treated as the sys tenant and create tables, causing conflicts with the background thread. If the background thread takes a long time to execute, it will result in a timeout when creating the tenant. To fix this issue, an error is immediately thrown when the account ID is not set in the context to avoid further errors. The code is then modified to pass in the account ID based on the error. This change affects all the places where the account ID is retrieved from the context. Although there are many changes, they are all straightforward. Changes in pkg/util/metric/mometric/metric.go:
Review:
Suggestions:
Here are review comments for file pkg/util/sysview/sysview.go: Title: cp to 1.1: fix create account timeout Body:
Changes in pkg/util/sysview/sysview.go:
Suggestions:
Overall, the pull request addresses an issue related to the create account timeout and makes changes to ensure that the account ID is properly set in the context. However, it is important to review the security implications of the changes and ensure that proper validations and sanitizations are in place. Additionally, providing more context and test cases would improve the clarity and effectiveness of the changes. Finally, improving the code readability through better variable and function names, as well as adding comments, would enhance the maintainability of the code. Here are review comments for file pkg/util/trace/impl/motrace/trace.go: Title: cp to 1.1: fix create account timeout Body:
Changes in trace.go:
Review comments:
Suggestions:
Here are review comments for file pkg/vm/engine/disttae/engine.go: Title: cp to 1.1: fix create account timeout Body:
Changes in
Overall, the changes made in this pull request address the issue of not setting the account ID in the context and provide a solution to avoid conflicts and timeouts during account creation. The modifications to pass the account ID based on the error message seem appropriate. However, there are a few suggestions for optimization and improvement:
Overall, the changes made in this pull request address the issue effectively. With the suggested optimizations and improvements, the codebase will be more robust, maintainable, and easier to understand. Here are review comments for file pkg/vm/engine/disttae/tools.go: Body:
Changes in pkg/vm/engine/disttae/tools.go:
Review:
Here are review comments for file pkg/vm/engine/disttae/txn.go: Body:
Changes in pkg/vm/engine/disttae/txn.go:
Review:
Suggestions:
Optimizations:
Overall, the pull request addresses the create account timeout issue and the changes made in the Here are review comments for file pkg/vm/engine/disttae/txn_table.go: Title: cp to 1.1: fix create account timeout Body:
Changes in txn_table.go:
Security Issues:
Suggestions for Optimization:
Overall, the pull request addresses the reported issue and makes the necessary changes to fix it. The changes seem straightforward and do not introduce any security issues. Here are review comments for file pkg/vm/engine/memoryengine/access_info.go: Title: cp to 1.1: fix create account timeout Body:
Changes in access_info.go:
Overall, the changes made in the pull request seem to address the issue of create account timeout by ensuring that the account ID is set in the context. The modifications in the
Suggestion: func getAccessInfo(ctx context.Context) (info AccessInfo, err error) {
info.AccountID, err = defines.GetAccountId(ctx)
if err != nil {
return info, err
}
info.UserID = defines.GetUserId(ctx)
info.RoleID = defines.GetRoleId(ctx)
return
}
Suggestion: func getAccessInfo(ctx context.Context) (info AccessInfo, err error) {
info.AccountID, err = defines.GetAccountId(ctx)
if err != nil {
return info, err
}
info.UserID = defines.GetUserId(ctx)
info.RoleID = defines.GetRoleId(ctx)
return
}By addressing these suggestions, the code will be more robust and optimized. Here are review comments for file pkg/vm/engine/memoryengine/compiler_context.go: Title: cp to 1.1: fix create account timeout Body:
Changes in compiler_context.go:
Review comments:
Here are review comments for file pkg/vm/engine/memoryengine/engine.go: Title: cp to 1.1: fix create account timeout Body: The pull request fixes an issue where the context passed to cn and tn does not include the account ID in certain cases. This causes a conflict with the background task thread, which defaults to the sys tenant. The fix is to immediately throw an error when the account ID is not set in the context, and then modify the code to pass the account ID based on the error. This change affects all the places where the account ID is retrieved from the context. Changes in pkg/vm/engine/memoryengine/engine.go:
Overall, the changes made in the pull request seem to address the issue of not including the account ID in the context in certain cases. However, there are a few suggestions for improvement:
Overall, the changes seem to address the issue, but there is room for improvement in terms of code optimization, error handling, security, testing, and documentation. Here are review comments for file pkg/vm/engine/tae/rpc/handle.go: Title: cp to 1.1: fix create account timeout Body:
Changes in handle.go:
Suggestions for improvement:
Here are review comments for file pkg/vm/engine/tae/txn/txnimpl/txn.go: Title: cp to 1.1: fix create account timeout Body: The pull request fixes an issue with the create account timeout. The problem was that in certain cases, the context passed to cn and tn did not include the account ID. This caused conflicts with the background task thread, which defaults to the sys tenant. The fix is to immediately throw an error if the account ID is not set in the context, and then modify the code to pass in the account ID based on the error. This change affects all places where the account ID is retrieved from the context. Changes in pkg/vm/engine/tae/txn/txnimpl/txn.go:
Overall, the changes made in this pull request seem to address the issue of not including the account ID in the context in certain cases. The modifications to the
In terms of security, there don't appear to be any obvious security issues introduced by these changes. However, it's always important to thoroughly test the code changes to ensure that they don't introduce any vulnerabilities or security risks. |
What type of PR is this?
Which issue(s) this PR fixes:
issue #13944
What this PR does / why we need it:
原因：
传给cn，tn的context，某些情况下没有带上accountid。原先没带上accountid，默认就是sys租户。
mo后台任务线程默认就是sys租户。创建普通租户时，普通租户会创建一些表。如果此时accountid没设置， 就会被当作sys租户，来创建一些表。会与后台线程冲突。如果后台线程执行时间长，表现为创建租户时卡住。
修改：
在context没有设置accountid时，立即报错。避免进一步出错。
后续根据报错，修改代码，传入accountid。
影响：
修改了所有从context取accountid的地方。改动较多位置。但是改动都很直观。