klesh commented on code in PR #5609:
URL:
https://github.com/apache/incubator-devlake/pull/5609#discussion_r1251453077
##########
backend/impls/dalgorm/dalgorm_transaction.go:
##########
@@ -48,6 +50,56 @@ func (t *DalgormTransaction) Commit() errors.Error {
return nil
}
+func (t *DalgormTransaction) LockTables(tables map[string]bool) errors.Error {
Review Comment:
This is a good idea, can you log it as an issue? I will do it later in
another PR
##########
backend/core/errors/util.go:
##########
@@ -28,3 +28,17 @@ func Is(err, target error) bool {
func As(err error, target any) bool {
return errors.As(err, &target)
}
+
+func Must(err error) {
Review Comment:
Not exactly, I think we should keep using the error-handling boilerplate
within a reasonably.
For errors that we are sure what was wrong, like 400 errors, e.g. records
not found, we should wrap ONCE and return it to users.
For errors that we are not sure what was wrong, we panic. for example, the
database went lost. we should panic and wrap the error ONCE with panic stack
trace on the gin level, tell users to take a screenshot,s and file issues on
the GitHub.
I believe we are using too much Wrap in our system.
Golang's convention is `Must`, check `regex` from stdlib
https://pkg.go.dev/regexp#MustCompile
##########
backend/helpers/pluginhelper/api/scope_generic_helper.go:
##########
@@ -297,7 +300,24 @@ func (gs *GenericScopeApiHelper[Conn, Scope, ScopeConfig])
GetScope(input *plugi
return scopeRes, nil
}
-func (gs *GenericScopeApiHelper[Conn, Scope, ScopeConfig]) DeleteScope(input
*plugin.ApiResourceInput) (*serviceHelper.BlueprintProjectPairs, errors.Error) {
+func (gs *GenericScopeApiHelper[Conn, Scope, ScopeConfig]) DeleteScope(input
*plugin.ApiResourceInput) (refs *serviceHelper.BlueprintProjectPairs, err
errors.Error) {
+ txHelper := dbhelper.NewTxHelper(gs.basicRes, &err)
+ defer txHelper.End()
+ tx := txHelper.Begin()
+ err = txHelper.LockTablesTimeout(2*time.Second,
map[string]bool{"_devlake_pipelines": false})
Review Comment:
Not really, the tables to be locked depends on contexts.
Sometimes we just need the `_devlake_pipelines`, sometimes we need the
`_devlake_pipeline_labels`, and others as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
