refactor: Refactor and unified error logging #363
Conversation
WalkthroughStandardizes error handling across multiple CLI command modules by converting function return types from integer exit codes to Go error types, introducing structured CraneError-based error handling with semantic codes and messages, and adding logger initialization across command packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
internal/cbatch/cmd.go (1)
120-120: Consequence of error handling changes above.This explicit
return nilensures exit code 0 even when errors occur. If you restore proper error propagation in the earlier segments, this line should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/cacct/cmd.go(3 hunks)internal/cacctmgr/cmd.go(1 hunks)internal/cbatch/cmd.go(4 hunks)internal/ccancel/cmd.go(3 hunks)internal/ccontrol/cmd.go(1 hunks)internal/ceff/cmd.go(3 hunks)internal/cinfo/cmd.go(3 hunks)internal/cqueue/cmd.go(3 hunks)internal/util/log.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
internal/ccancel/cmd.go (2)
internal/ccancel/ccancel.go (1)
CancelTask(35-107)internal/util/log.go (1)
CraneFormatter(26-28)
internal/cqueue/cmd.go (4)
internal/util/err.go (1)
NewCraneErr(60-65)internal/cinfo/cmd.go (1)
FlagIterate(34-34)internal/cqueue/cqueue.go (1)
Query(79-88)internal/util/log.go (1)
CraneFormatter(26-28)
internal/cinfo/cmd.go (3)
internal/cqueue/cmd.go (1)
FlagIterate(43-43)internal/cqueue/cqueue.go (1)
Query(79-88)internal/util/log.go (1)
CraneFormatter(26-28)
internal/cbatch/cmd.go (3)
internal/util/err.go (2)
NewCraneErr(60-65)WrapCraneErr(67-73)internal/cbatch/cbatch.go (3)
BuildCbatchJob(45-333)SendRequest(335-363)SendMultipleRequests(365-398)internal/util/log.go (1)
CraneFormatter(26-28)
internal/ceff/cmd.go (2)
internal/ceff/ceff.go (1)
QueryTasksInfoByIds(415-487)internal/util/log.go (1)
CraneFormatter(26-28)
internal/cacct/cmd.go (3)
internal/util/err.go (2)
NewCraneErr(60-65)ErrorCmdArg(36-36)internal/cacct/cacct.go (1)
QueryJob(48-343)internal/util/log.go (1)
CraneFormatter(26-28)
internal/cacctmgr/cmd.go (1)
internal/util/log.go (1)
CraneFormatter(26-28)
internal/ccontrol/cmd.go (1)
internal/util/log.go (1)
CraneFormatter(26-28)
🔇 Additional comments (9)
internal/cacctmgr/cmd.go (1)
927-929: LGTM! Logging formatter initialization is appropriate.The init function correctly sets up the CraneFormatter for consistent log output across the application.
internal/cinfo/cmd.go (1)
119-119: LGTM! Logging formatter initialization is appropriate.internal/ccontrol/cmd.go (1)
498-500: LGTM! Logging formatter initialization is appropriate.The init function correctly sets up the CraneFormatter for consistent log output.
internal/ccancel/cmd.go (1)
115-115: LGTM! Logging formatter initialization is appropriate.internal/ceff/cmd.go (1)
78-78: LGTM! Logging formatter initialization is appropriate.internal/cacct/cmd.go (1)
167-167: LGTM! Logging formatter initialization is appropriate.internal/cqueue/cmd.go (1)
163-163: LGTM! Logging formatter initialization is appropriate.internal/cbatch/cmd.go (2)
28-28: LGTM: Logrus import added for structured logging.The import is necessary for the logging functionality introduced in this refactor.
164-164: Formatter setup looks appropriate, but verify the logging approach aligns with project goals.Setting the global log formatter in
init()is standard practice and ensures consistent log formatting before any logging occurs. TheCraneFormatteris used consistently across command packages according to the PR summary.However, this change is part of a broader refactoring that fundamentally alters error handling patterns (see previous comments). Before merging, verify that:
- The team intends to use logging instead of error returns throughout the codebase
- Exit code handling is addressed (currently broken by returning
nilon errors)- The simplified log format (message-only) provides sufficient context for debugging
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/cacct/cmd.go(1 hunks)internal/cacctmgr/cacctmgr.go(28 hunks)internal/cacctmgr/cmd.go(33 hunks)internal/cbatch/cmd.go(1 hunks)internal/ccancel/cmd.go(1 hunks)internal/ccontrol/ccontrol.go(1 hunks)internal/ccontrol/cmd.go(12 hunks)internal/ceff/cmd.go(2 hunks)internal/cinfo/cmd.go(1 hunks)internal/cqueue/cmd.go(1 hunks)internal/util/log.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/ceff/cmd.go
- internal/cqueue/cmd.go
- internal/cinfo/cmd.go
- internal/cacct/cmd.go
- internal/util/log.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-10T14:48:45.121Z
Learnt from: MCKZX-llx
Repo: PKUHPC/CraneSched-FrontEnd PR: 281
File: internal/util/raft.go:10-10
Timestamp: 2025-06-10T14:48:45.121Z
Learning: 在 CraneSched-FrontEnd 项目中，前端命令工具采用单进程单线程架构，每个命令执行时不会有并发访问全局变量的问题。多进程间对持久化文件的访问通过文件锁（如 github.com/gofrs/flock）来实现互斥。因此，全局变量不需要额外的mutex保护。
Applied to files:
internal/cbatch/cmd.go
🧬 Code graph analysis (6)
internal/ccontrol/ccontrol.go (1)
internal/util/err.go (2)
CraneError(44-48)ErrorBackend(39-39)
internal/cbatch/cmd.go (1)
internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cacctmgr/cacctmgr.go (7)
internal/util/err.go (6)
CraneError(44-48)ErrorSuccess(35-35)ErrorInvalidFormat(40-40)ErrorCmdArg(37-37)ErrorBackend(39-39)ErrorNetwork(38-38)internal/cacctmgr/cmd.go (2)
FlagForce(62-62)FlagJson(64-64)internal/util/string.go (2)
CheckEntityName(564-584)ParseStringParamList(1087-1096)internal/util/formatter.go (1)
FmtJson(58-58)internal/util/os.go (1)
GetUidByUserName(168-182)internal/util/util.go (2)
Config(25-39)InfluxDbConfig(61-68)internal/util/grpc.go (1)
GrpcErrorPrintf(387-398)
internal/ccancel/cmd.go (1)
internal/util/log.go (1)
InitCraneLogger(35-37)
internal/ccontrol/cmd.go (2)
internal/util/err.go (4)
CraneError(44-48)ErrorSuccess(35-35)ErrorCmdArg(37-37)ErrorGeneric(36-36)internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cacctmgr/cmd.go (4)
internal/util/err.go (3)
CraneError(44-48)ErrorCmdArg(37-37)ErrorSuccess(35-35)internal/cacctmgr/parser.go (1)
CAcctMgrCommand(31-33)internal/cacctmgr/cacctmgr.go (10)
AddAccount(386-448)AddUser(450-521)AddQos(523-565)DeleteUser(616-669)BlockAccountOrUser(1114-1168)UnblockAccountOrUser(1170-1224)ModifyAccount(719-794)ModifyUser(796-870)ModifyQos(872-909)ShowTxn(1226-1290)internal/util/log.go (1)
InitCraneLogger(35-37)
🔇 Additional comments (9)
internal/cbatch/cmd.go (1)
156-156: LGTM! Logger initialization follows project-wide pattern.The addition of
util.InitCraneLogger()in the init() function is consistent with the same change across other command packages and ensures the Crane logger formatter is properly initialized before command execution.internal/ccancel/cmd.go (1)
111-111: LGTM! Logger initialization follows project-wide pattern.Consistent with other command packages in this PR.
internal/ccontrol/cmd.go (3)
23-24: LGTM! Import additions support the error refactoring.The
errorsandfmtpackages are properly used for CraneError handling and formatted error messages throughout the file.
80-91: LGTM! Error handling pattern correctly implemented.The error handling logic properly distinguishes between success messages (printed via
fmt.Print) and errors (logged vialog.Error), and exits with the appropriate code fromCraneError.Code.
568-570: LGTM! Logger initialization follows project-wide pattern.Consistent with other command packages in this PR.
internal/cacctmgr/cacctmgr.go (1)
233-268: LGTM! Error-based return pattern consistently implemented.The refactoring from numeric exit codes to
CraneError-based returns is systematic and well-executed across all modified functions. Error codes (ErrorSuccess,ErrorCmdArg,ErrorNetwork,ErrorBackend) are appropriately assigned based on the failure mode, and messages provide clear context.Also applies to: 386-448, 911-949
internal/cacctmgr/cmd.go (3)
24-25: LGTM! Import additions and error handling pattern match ccontrol.The error handling logic at lines 152-163 correctly uses
errors.Asto detectCraneError, distinguishes between success messages and errors, and exits with appropriate codes. Consistent with the pattern ininternal/ccontrol/cmd.go.Also applies to: 152-163
166-194: LGTM! Command execution functions consistently refactored to error-based returns.All
execute*functions now returnerrorwith appropriateCraneErrorconstruction. Validation errors useErrorCmdArg, and messages provide clear context. The pattern is uniform across all command handlers.Also applies to: 196-1068
1070-1072: LGTM! Logger initialization follows project-wide pattern.Consistent with other command packages in this PR.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ccontrol/cmd.go (1)
263-338: Error accumulation pattern loses intermediate error codes.The function pre-allocates a
CraneErrorwithErrorSuccessand accumulates errors by mutating theCodefield and appending toMessage. This pattern has issues:
- If multiple operations fail with different error codes, only the last error code is preserved (e.g., if priority update fails with
ErrorCmdArgand time limit fails withErrorGeneric, the returned code isErrorGeneric).- The
Codefield doesn't accurately represent the combined failure state when multiple operations fail.- Pre-allocating with
ErrorSuccessand mutating is non-idiomatic Go error handling.Consider one of these approaches:
Option 1: Return on first error instead of accumulating:
if jobParamFlags&PriorityTypeFlag != 0 { // ... parse priority ... err = ChangeTaskPriority(FlagTaskIds, FlagPriority) if err != nil { - craneError.Message += fmt.Sprintf("change task priority failed: %s\n", err) - craneError.Code = util.ErrorGeneric + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return craneErr + } + return &util.CraneError{ + Code: util.ErrorGeneric, + Message: fmt.Sprintf("change task priority failed: %s\n", err), + } } }Option 2: Collect all errors and use a consistent code for "partial failure":
- var craneError = &util.CraneError{ - Code: util.ErrorSuccess, - Message: "", - } + var errorMessages []string + hasError := false if jobParamFlags&PriorityTypeFlag != 0 { // ... parse priority ... err = ChangeTaskPriority(FlagTaskIds, FlagPriority) if err != nil { - craneError.Message += fmt.Sprintf("change task priority failed: %s\n", err) - craneError.Code = util.ErrorGeneric + errorMessages = append(errorMessages, fmt.Sprintf("change task priority failed: %s", err)) + hasError = true } } // ... similar for other operations ... - return craneError + if hasError { + return &util.CraneError{ + Code: util.ErrorGeneric, + Message: strings.Join(errorMessages, "\n") + "\n", + } + } + return &util.CraneError{Code: util.ErrorSuccess}
🧹 Nitpick comments (1)
internal/ccontrol/cmd.go (1)
130-191: Consider more specific error codes for different failure modes.All failures in the show commands (nodes, partitions, jobs, reservations) are currently mapped to
ErrorGeneric. While the error messages are preserved, consider whether different failure modes (e.g., network errors vs. not-found vs. permission denied) should use different error codes for better programmatic error handling.For example, if
ShowNodesand similar functions can distinguish between network failures and other issues, propagate those as differentCraneErrorcodes rather than uniformly usingErrorGeneric.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ccontrol/cmd.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/ccontrol/cmd.go (5)
internal/ccontrol/parser.go (1)
CControlCommand(33-35)internal/util/err.go (4)
CraneError(44-48)ErrorSuccess(35-35)ErrorCmdArg(37-37)ErrorGeneric(36-36)internal/ccontrol/output.go (4)
ShowNodes(262-275)ShowPartitions(359-372)ShowJobs(706-723)ShowReservations(468-481)internal/ccontrol/ccontrol.go (8)
ChangeNodeState(313-365)ChangeTaskPriority(180-223)ChangeTaskTimeLimit(93-128)ChangeTaskExtraAttrs(225-311)ModifyPartitionAcl(367-400)HoldReleaseJobs(130-178)CreateReservation(402-479)DeleteReservation(481-510)internal/util/log.go (1)
InitCraneLogger(35-37)
🔇 Additional comments (5)
internal/ccontrol/cmd.go (5)
23-24: LGTM: Import additions support error handling refactor.The
errorsandfmtpackages are appropriately used forerrors.Astype assertions and formatted error messages throughout the refactored code.
80-91: Verify the success-with-message pattern.The logic at lines 83-84 prints messages for
CraneErrorwithErrorSuccesscode. This pattern is unusual—typically, successful operations either return nil or use standard output directly rather than packaging messages in an error structure with a success code.Confirm whether this pattern is intentional and document its purpose. Consider whether success messages should be emitted directly by the command handlers instead.
94-108: LGTM: Function signature and error handling updated correctly.The refactored error handling appropriately returns
CraneErrorwithErrorCmdArgfor unknown operations.
534-566: LGTM: Parameter validation refactored cleanly.The function correctly validates parameters and returns appropriate
CraneErrorinstances withErrorCmdArgfor validation failures. The nil return on success follows Go conventions.
568-570: LGTM: Logger initialization added.The
init()function properly initializes the Crane logger, aligning with the refactoring objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cacctmgr/cacctmgr.go (2)
719-792: Inconsistent success return pattern.The
Modify*functions (ModifyAccount,ModifyUser,ModifyQos) returnnilon success, which is correct Go convention. However, most other functions in this file return&util.CraneError{Code: util.ErrorSuccess, Message: msg}on success. This inconsistency forces callers to handle two different success patterns.Recommend standardizing on
nilfor all success cases throughout the file.Also applies to: 794-866, 868-903
1287-1335: Returnnilon success, not a success-coded error.Line 1334 returns
&util.CraneError{Code: util.ErrorSuccess}as the error return value. This violates Go's error-handling convention wherenilindicates success. Callers expectingerr == nilfor success will incorrectly treat this as an error.Apply this diff:
- return dbConf.Database, &util.CraneError{Code: util.ErrorSuccess} + return dbConf.Database, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cacctmgr/cacctmgr.go(28 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacctmgr/cacctmgr.go (6)
internal/util/err.go (7)
CraneError(44-48)ErrorSuccess(35-35)ErrorInvalidFormat(40-40)ErrorCmdArg(37-37)ErrorBackend(39-39)ErrMsg(206-211)ErrorNetwork(38-38)internal/util/string.go (2)
CheckEntityName(564-584)ParseStringParamList(1087-1096)internal/util/formatter.go (1)
FmtJson(58-58)internal/util/os.go (1)
GetUidByUserName(168-182)internal/util/util.go (2)
Config(25-39)InfluxDbConfig(61-68)internal/util/grpc.go (1)
GrpcErrorPrintf(387-398)
|
Rebase 一下，需要按 #370 的做法重构一下。 |
edragain2nd
force-pushed
the
refactor/error
branch
from
225782a to
5e66b96
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
internal/ccontrol/cmd.go (4)
233-237: Error code mapping needs correction.The error from
ChangeNodeStateis wrapped withErrorCmdArg, but based on the relevant code snippets,ChangeNodeStatereturnsutil.CraneErrorwith codes likeErrorNetworkfor network failures orErrorBackendfor backend issues. Wrapping withErrorCmdArgloses this specificity.This was flagged in a previous review. Since
ChangeNodeStatealready returns autil.CraneError, consider propagating it directly or preserving its code.Apply this diff to preserve the original error code:
err := ChangeNodeState(FlagNodeName, FlagState, FlagReason) if err.Code != util.ErrorSuccess { - return *util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("change node state failed: %s\n", err.Message)) + return *util.NewCraneErr(err.Code, fmt.Sprintf("change node state failed: %s\n", err.Message)) } return *util.NewCraneErr(util.ErrorSuccess, "")
323-330: Preserve original error codes from ModifyPartitionAcl.As flagged in previous reviews,
ModifyPartitionAclreturnsutil.CraneErrorwith specific codes. Wrapping withErrorGenericloses this information.Apply this diff:
case "accounts", "allowedaccounts": FlagAllowedAccounts = value if err := ModifyPartitionAcl(FlagPartitionName, true, FlagAllowedAccounts); err.Code != util.ErrorSuccess { - return *util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("%s\n", err.Message)) + return *util.NewCraneErr(err.Code, fmt.Sprintf("%s\n", err.Message)) } case "deniedaccounts": FlagDeniedAccounts = value if err := ModifyPartitionAcl(FlagPartitionName, false, FlagDeniedAccounts); err.Code != util.ErrorSuccess { - return *util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("%s\n", err.Message)) + return *util.NewCraneErr(err.Code, fmt.Sprintf("%s\n", err.Message)) }
356-360: Preserve original error codes from HoldReleaseJobs.As flagged in previous reviews, the error code from
HoldReleaseJobsshould be preserved.Apply this diff:
err := HoldReleaseJobs(jobIds, true) if err.Code != util.ErrorSuccess { - return *util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("hold jobs failed: %s\n", err.Message)) + return *util.NewCraneErr(err.Code, fmt.Sprintf("hold jobs failed: %s\n", err.Message)) }Apply the same pattern to
executeReleaseCommand(lines 369-373).
423-426: Preserve original error codes from CreateReservation and DeleteReservation.As flagged in previous reviews, the error codes should be preserved.
Apply this diff for
executeCreateReservationCommand:err := CreateReservation() if err.Code != util.ErrorSuccess { - return *util.NewCraneErr(util.ErrorGeneric, err.Message) + return *util.NewCraneErr(err.Code, err.Message) }Apply the same pattern to
executeDeleteReservationCommand(lines 447-449).
🧹 Nitpick comments (3)
internal/util/grpc.go (1)
405-422: Consider handling non-gRPC errors more explicitly.The function returns an empty string when the error is not a gRPC status. This could lead to empty error messages being passed to
NewCraneErr. Looking at usage ininternal/cacctmgr/cacctmgr.go(line 948), if the error is not a gRPC error, theCraneErrorwill have an empty message.Consider returning the formatted string with the original error message as a fallback:
func GrpcErrorSprintf(err error, format string, a ...any) string { s := fmt.Sprintf(format, a...) if rpcErr, ok := grpcstatus.FromError(err); ok { switch rpcErr.Code() { case grpccodes.Unavailable: return fmt.Sprintf("%s: Connection to CraneCtld is broken.", s) case grpccodes.Unauthenticated: return fmt.Sprintf("%s: Access denied.", s) case grpccodes.DeadlineExceeded: return fmt.Sprintf("%s: Request timeout, please try again or reduce the query scope.", s) case grpccodes.ResourceExhausted: return fmt.Sprintf("%s: Response too large, please reduce the query scope.", s) default: return fmt.Sprintf("%s: gRPC error code %s.", s, rpcErr.String()) } } - return "" + return fmt.Sprintf("%s: %v", s, err) }internal/ccontrol/ccontrol.go (1)
508-508: Inconsistent trailing newline in error message.This error message has a trailing
\nwhile similar messages (e.g., line 480) do not. For consistency, remove the trailing newline.- return *util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.\n", reply.GetReason())) + return *util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.", reply.GetReason()))internal/cacctmgr/cmd.go (1)
159-159: Minor: Inconsistent trailing newlines in error messages.Error messages like
"unknown entity type: %s\n"include trailing newlines. Sincelog.Errortypically appends a newline, this may cause double newlines in output. Consider removing trailing\nfrom error messages for consistency.This is a minor formatting concern and doesn't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
internal/cacct/cmd.go(1 hunks)internal/cacctmgr/cacctmgr.go(19 hunks)internal/cacctmgr/cmd.go(34 hunks)internal/cacctmgr/output.go(16 hunks)internal/cbatch/cmd.go(1 hunks)internal/ccancel/cmd.go(1 hunks)internal/ccontrol/ccontrol.go(16 hunks)internal/ccontrol/cmd.go(11 hunks)internal/ccontrol/output.go(10 hunks)internal/ceff/cmd.go(2 hunks)internal/cinfo/cmd.go(1 hunks)internal/cqueue/cmd.go(1 hunks)internal/util/err.go(3 hunks)internal/util/grpc.go(1 hunks)internal/util/log.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/cqueue/cmd.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cbatch/cmd.go
- internal/util/log.go
- internal/ceff/cmd.go
🧰 Additional context used
🧬 Code graph analysis (6)
internal/ccancel/cmd.go (1)
internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cinfo/cmd.go (1)
internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cacct/cmd.go (1)
internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cacctmgr/cacctmgr.go (2)
internal/util/err.go (6)
CraneError(44-48)NewCraneErr(61-66)ErrorSuccess(35-35)ErrorBackend(39-39)ErrMsg(208-213)ErrorNetwork(38-38)internal/util/string.go (1)
CheckEntityName(609-629)
internal/ccontrol/ccontrol.go (4)
internal/util/err.go (7)
CraneError(44-48)NewCraneErr(61-66)ErrorBackend(39-39)ErrorSuccess(35-35)ErrorGeneric(36-36)ErrorCmdArg(37-37)ErrorNetwork(38-38)internal/util/string.go (1)
ConvertSliceToString(1258-1264)internal/util/grpc.go (1)
GrpcErrorSprintf(405-422)internal/util/formatter.go (1)
FmtJson(58-58)
internal/ccontrol/cmd.go (4)
internal/util/err.go (5)
CraneError(44-48)NewCraneErr(61-66)ErrorCmdArg(37-37)ErrorSuccess(35-35)ErrorGeneric(36-36)internal/ccontrol/output.go (1)
ShowNodes(256-270)internal/ccontrol/ccontrol.go (1)
ChangeNodeState(316-368)internal/util/log.go (1)
InitCraneLogger(35-37)
🔇 Additional comments (21)
internal/cinfo/cmd.go (1)
113-113: LGTM!The
util.InitCraneLogger()call is correctly placed at the end ofinit(), consistent with the pattern across other command packages. The error propagation viaRunEis properly handled byRunAndHandleExit.internal/ccancel/cmd.go (1)
105-105: LGTM!Logger initialization is correctly added. The
RunEhandler properly returns the error fromCancelTask, which will be handled byRunAndHandleExitfor appropriate exit codes.internal/util/err.go (2)
88-96: LGTM - Proper error silencing logic.The logic correctly handles different error scenarios:
- For
ErrorCmdArg: Usage info is shown (helpful for argument errors), Cobra's default error output is used- For other errors: Both usage and Cobra's error output are silenced since
RunAndHandleExithandles logging
112-114: LGTM - Conditional logging before exit.The error is logged only when there's a non-empty message and it's not a command argument error (which Cobra handles). This prevents duplicate error messages and ensures clean output.
internal/cacct/cmd.go (1)
163-163: LGTM!Logger initialization follows the established pattern. The
RunEhandler correctly returns errors which are handled by the wrapper functions.internal/cacctmgr/cacctmgr.go (2)
93-141: Consistent pattern for CraneError-based returns.The refactored functions consistently use
*util.NewCraneErr(code, message)pattern and returnutil.CraneErrorby value. This is a valid design choice that allows unified error handling. The success paths usingErrorSuccesswith empty messages work correctly with theRunAndHandleExitlogic.
676-676: I'm unable to proceed with verification because the repositoryPKUHPC/CraneSched-FrontEndis not accessible in the sandboxed environment. The clone operation failed, which prevents me from:
- Examining the
GetEventPluginConfigfunction definition- Inspecting the
CraneErrortype definition- Analyzing all callers and their error handling patterns
- Confirming whether the concern about return-by-value vs pointer semantics is accurate
The review comment makes specific technical claims that require access to the actual codebase to verify. Without being able to inspect the code, I cannot determine if:
GetEventPluginConfigreturnsCraneErrorby value or as a pointer- Line 676 is inside the function or a caller
- All callers properly check
err.Coderather thanerr != nil
Verify success return pattern from GetEventPluginConfig.
GetEventPluginConfigreturns a CraneError withErrorSuccesson success. The caller at line 676 must checkerr.Code != ErrorSuccessrather thanerr != nilsince this function returns by value.Ensure all callers handle this correctly. If they expect pointer semantics (checking
err != nil), this will cause issues.internal/ccontrol/ccontrol.go (2)
50-94: Good refactoring of SummarizeReply to return CraneError.The function now properly aggregates failure messages and returns structured errors. The pattern of returning
ErrorSuccesson complete success andErrorBackendwith accumulated messages on partial failures is well-implemented.
96-131: Proper use of GrpcErrorSprintf for network errors.The function correctly uses
util.GrpcErrorSprintfto format gRPC errors with context, providing better error messages to users.internal/cacctmgr/cmd.go (3)
103-109: Inconsistent success return pattern.The
validateUintValuefunction returns a CraneError with an empty message on success. This pattern is repeated throughout the file but could be simplified by returning a pointer and usingnilfor success, which is more idiomatic in Go error handling.However, since this aligns with the project's CraneError-based pattern (returning value types), this is acceptable. The implementation is correct.
127-131: LGTM!The error handling correctly logs the message only when non-empty and exits with the appropriate code. This is the expected pattern for the CraneError-based flow.
901-903: LGTM!The
init()function correctly callsutil.InitCraneLogger()as a centralized approach, which addresses the previous reviewer's suggestion to use a function from the util package.internal/cacctmgr/output.go (2)
143-174: LGTM!The function correctly handles the empty list case and propagates the CraneError from
PrintAccountTable.
273-304: LGTM!The
ShowAccountsfunction correctly handles network errors, JSON output, and backend error aggregation. The error message building for rich errors is appropriate.internal/ccontrol/cmd.go (1)
481-483: LGTM!The
init()function correctly initializes the Crane logger using the centralized function.internal/ccontrol/output.go (6)
145-152: LGTM!The
getCranedNodesReplyfunction correctly returns a CraneError for network failures and success. The use ofutil.GrpcErrorSprintffor formatting gRPC errors is appropriate.
161-165: LGTM!The
outputNodesfunction correctly returns void since it only performs printing operations that cannot fail. The iteration over nodes and callingprintNodeDetailsis straightforward.
508-519: LGTM!The
outputJobsfunction correctly propagates errors fromprintJobDetailsand handles the printed job tracking. The call tocheckMissingJobscorrectly ignores the return since it's void.
562-579: LGTM!The
getUserGroupInfofunction correctly handles user and group lookup failures by returning appropriate CraneError values. UsingErrorGenericfor system-level failures is reasonable.
694-711: LGTM!The
ShowJobsfunction correctly handles the full flow: parsing job IDs, fetching task info, handling JSON output, and propagating errors from nested calls.
713-750: LGTM!The
ShowLicensesfunction correctly handles all cases: network errors, backend errors, JSON output, empty results, and successful output. The error handling pattern is consistent with other Show* functions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/ccontrol/cmd.go (1)
246-312: Priority parse inexecuteUpdateJobCommandshould fail fastIn
executeUpdateJobCommand, whenstrconv.ParseFloatfails:priority, err := strconv.ParseFloat(value, 64) if err != nil { craneError.Code = util.ErrorCmdArg } FlagPriority = priority result := ChangeTaskPriority(FlagTaskIds, FlagPriority)The code only sets an error code but continues execution, calling
ChangeTaskPrioritywithpriorityat its zero value (0). This silently applies an unintended priority instead of surfacing a clear error to the user.Per Go CLI best practices, return early on parse failure:
priority, err := strconv.ParseFloat(value, 64) if err != nil { - craneError.Code = util.ErrorCmdArg -} -FlagPriority = priority -result := ChangeTaskPriority(FlagTaskIds, FlagPriority) + return util.NewCraneErr(util.ErrorCmdArg, + fmt.Sprintf("invalid priority value: %s\n", value)) +} +FlagPriority = priority +result := ChangeTaskPriority(FlagTaskIds, FlagPriority)internal/cacctmgr/cacctmgr.go (1)
553-641: Block/unblock flows use anti-idiomatic error handling
BlockAccountOrUserandUnblockAccountOrUserreturnutil.NewCraneErr(util.ErrorSuccess, "")on success rather thannil. In Go, the standard idiom is to returnnilfor success and reserve non-nil error returns for failures only. Returning a non-nil error to signal success violates caller expectations (callers checkerr != nilto detect failures). Migrate these functions to returnnilon success to align with Go conventions and improve consistency with other functions likeModifyAccountandFindAccount.
♻️ Duplicate comments (7)
internal/cacctmgr/output.go (2)
89-141:ShowTxnJSON handling now avoids logging JSON as an error
ShowTxnnow:
- Validates
actionand time interval withErrorCmdArg.- On JSON + success, prints the JSON (
fmt.Print(msg)) and returnsErrorSuccesswith an empty message, avoiding logging JSON via the central error logger.- On backend failure, returns
ErrorBackendwith the JSON or error string.This resolves the earlier pattern where JSON was carried in the error message and logged as an error.
143-176:PrintAccountListcorrectly returnsnilon success/empty resultsFor empty lists it now returns
nil(no spurious success error), and for non‑empty lists it prints the tree then delegates toPrintAccountTable, propagating any error. This aligns with theerrorreturn type and prior review feedback.internal/cacctmgr/cmd.go (1)
713-790: Duplicate ofModifyQossuccess bug: all QoS modifications will be treated as failuresIn
executeModifyQosCommand, every QoS field update wraps:if err := ModifyQos(...); err != nil { return util.WrapCraneErr(util.ErrorCmdArg, "%s\n", err) }Given the current
ModifyQosimplementation returnsutil.NewCraneErr(util.ErrorSuccess, "")on success,erris nevernil, so all modifications are reported as failures and wrapped asErrorCmdArg. FixingModifyQosto returnnilon success (see earlier comment incacctmgr.go) will resolve this; no changes here are needed after that.internal/ccontrol/cmd.go (4)
57-65: SameParseCmdArgsrobustness concern ascacctmgr
ParseCmdArgsinccontrolmirrors the pattern fromcacctmgr: it assumes any non‑nilresultis a*util.CraneErrorand dereferences without checkingerrors.As’s return value. Please apply the same guard/fallback pattern here as suggested forcacctmgr/cmd.goto avoid panics if future code returns a non‑CraneError.Does Go's `errors.As` set the target pointer to nil when no matching type is found, requiring an explicit boolean check?Also applies to: 80-91
93-187: Preserve originalCraneErrorcodes instead of re‑wrapping asErrorGenericThe show commands (
executeShowNodeCommand,executeShowPartitionCommand,executeShowJobCommand,executeShowReservationCommand,executeShowLicenseCommand) all:err := ShowX(...) if err != nil { return util.WrapCraneErr(util.ErrorGeneric, "show ... failed: %s\n", err) } return util.NewCraneErr(util.ErrorSuccess, "")But
ShowXalready returns well‑classified*util.CraneError(e.g.ErrorNetwork,ErrorBackend). Wrapping everything intoErrorGenericloses that information and breaks exit‑code semantics for callers.Prefer preserving the original code when possible:
err := ShowNodes(name, FlagQueryAll) if err != nil { - return util.WrapCraneErr(util.ErrorGeneric, "show nodes failed: %s\n", err) + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return util.NewCraneErr(craneErr.Code, + fmt.Sprintf("show nodes failed: %s\n", craneErr.Message)) + } + return util.WrapCraneErr(util.ErrorGeneric, "show nodes failed: %s\n", err) } -return util.NewCraneErr(util.ErrorSuccess, "") +return nilApply the same pattern to partitions/jobs/reservations/licenses.
#!/bin/bash # Inspect Show* implementations to confirm they already classify errors. rg -n "func ShowNodes" --type go -C3 rg -n "func ShowPartitions" --type go -C3 rg -n "func ShowJobs" --type go -C3 rg -n "func ShowReservations" --type go -C3 rg -n "func ShowLicenses" --type go -C3
189-215: Update-node command should preserve underlying error code
executeUpdateNodeCommandwraps all errors fromChangeNodeStateasErrorCmdArg:err := ChangeNodeState(...) if err != nil { return util.WrapCraneErr(util.ErrorCmdArg, "change node state failed: %s\n", err) }But
ChangeNodeStateitself likely returns*util.CraneErrorwith codes likeErrorNetwork/ErrorBackend. Wrapping asErrorCmdArgmisclassifies runtime failures as argument errors.Use the same pattern as for the
Show*commands: preserve existingCraneError.Codewhen possible and only fall back toErrorGenericfor unexpected error types.#!/bin/bash # Confirm ChangeNodeState returns *util.CraneError and inspect its code usage. rg -n "func ChangeNodeState" --type go -C5Also applies to: 216-244
314-346: Preserve original codes when wrappingModifyPartitionAcl,HoldReleaseJobs,Create/DeleteReservationerrorsThese command functions:
executeUpdatePartitionCommand(Lines 314–346)executeHoldCommand/executeReleaseCommand(Lines 348–380)executeCreateReservationCommand(Lines 392–434)executeDeleteReservationCommand(Lines 446–457)all wrap errors from lower‑level helpers as
ErrorGeneric, e.g.:if err := ModifyPartitionAcl(...); err != nil { return util.WrapCraneErr(util.ErrorGeneric, "%s\n", err) }If those helpers already return
*util.CraneErrorwith specific codes (network/backend), this again discards useful exit‑code information.Recommend using the same
errors.Aspattern as forShow*commands to preserve existingCraneError.Codewhere present and only fall back toErrorGenericfor unexpected error types.#!/bin/bash # Inspect helper implementations to confirm they already return *util.CraneError with specific codes. rg -n "ModifyPartitionAcl\(" --type go -C5 rg -n "HoldReleaseJobs\(" --type go -C5 rg -n "CreateReservation\(" --type go -C5 rg -n "DeleteReservation\(" --type go -C5Also applies to: 348-380, 392-457
🧹 Nitpick comments (6)
internal/cacctmgr/cacctmgr.go (1)
797-884:QueryEventInfoByNodeserror classification and JSON/text flows look consistent
QueryEventInfoByNodesnow:
- Treats invalid node patterns, missing nodes, empty
ClusterName, and bad sort asErrorCmdArg.- Wraps InfluxDB or sort failures as
ErrorBackend.- Prints JSON or table output on success and returns
ErrorSuccessas aCraneError.The logic and error codes are coherent; only remaining nit is the use of
ErrorSuccessas anerror(see earlier comment).internal/cacctmgr/output.go (2)
250-305: Account table printing and ShowAccounts integration are consistent
PrintAccountTableandShowAccountsnow:
- Use
AccountFormatOutputonly whenFlagFormatis set.- Print JSON then return
ErrorSuccesswith empty message in JSON mode.- Aggregate backend rich errors and wrap as
ErrorBackend.- Delegate to
PrintAccountListfor text output.Only stylistic nit is again returning
ErrorSuccessinstead ofnilon some success paths; otherwise behaviour is coherent.
549-613: ShowUser/ShowQos JSON and backend handling consistent with new patternBoth
ShowUserandShowQos:
- Validate CLI lists and wrap parse errors as
ErrorCmdArgusingWrapCraneErr.- On JSON + success, print JSON and return an
ErrorSuccesswith empty message.- Aggregate backend rich errors into a string and return
ErrorBackend.- Delegate to
PrintUserList/PrintQosListfor text output.Behaviour is consistent with the
cacctmgrrefactor; only remaining nit is success‑as‑error mentioned earlier.Also applies to: 753-794
internal/cacctmgr/cmd.go (3)
480-599: Modify‑account orchestration is correct, but returnsErrorSuccessinstead ofnil
executeModifyAccountCommandcorrectly:
- Requires a
whereclause withname.- Requires at least one
set/add/deleteclause.- Dispatches multiple
ModifyAccountRPCs based on actions.The only nit is returning
util.NewCraneErr(util.ErrorSuccess, "")at the end; consider switching this tonilfor success to match the updatedModifyAccountimplementation.
602-711: Modify‑user orchestration is consistent but uses success‑as‑error
executeModifyUserCommandusescheckEmptyKVParams, validates allowedset/add/deletekeys, and delegates toModifyUserwith appropriateOperationType. This matches the design inModifyUser. As with accounts, consider returningnilinstead ofErrorSuccessat the end.
880-906:checkEmptyKVParamserror messages are fine, but quoting is slightly awkward
checkEmptyKVParamscorrectly:
- Rejects empty
kvParamswhen required.- Produces specific messages for single vs multiple missing fields.
Minor nit: the multi‑field message uses
strings.Join(missingFields, "\", \""), which yields output likeError: required arguments field1", "field2 not set. If you care about aesthetics, consider wrapping the entire join in quotes or dropping them altogether.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/cacctmgr/cacctmgr.go(19 hunks)internal/cacctmgr/cmd.go(34 hunks)internal/cacctmgr/output.go(16 hunks)internal/ccontrol/ccontrol.go(13 hunks)internal/ccontrol/cmd.go(11 hunks)internal/ccontrol/output.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ccontrol/ccontrol.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T03:17:51.694Z
Learnt from: 1daidai1
Repo: PKUHPC/CraneSched-FrontEnd PR: 285
File: internal/cinfo/cinfo.go:363-364
Timestamp: 2025-06-12T03:17:51.694Z
Learning: 在 cinfo 命令中，使用 -o/--format 自定义输出时，应省略节点数为 0 的分区，不予显示。
Applied to files:
internal/ccontrol/output.go
🧬 Code graph analysis (2)
internal/cacctmgr/output.go (4)
internal/util/err.go (5)
NewCraneErr(61-66)WrapCraneErr(68-74)ErrorSuccess(35-35)ErrorBackend(39-39)ErrMsg(208-213)internal/cacctmgr/cmd.go (1)
FlagJson(64-64)internal/util/formatter.go (1)
FmtJson(58-58)internal/util/table.go (2)
FormatTable(51-96)SetBorderTable(41-49)
internal/ccontrol/output.go (3)
internal/util/err.go (5)
NewCraneErr(61-66)ErrorBackend(39-39)ErrorSuccess(35-35)WrapCraneErr(68-74)ErrorCmdArg(37-37)internal/util/grpc.go (1)
GrpcErrorSprintf(405-422)internal/util/string.go (1)
ParseJobIdList(1143-1155)
🔇 Additional comments (21)
internal/cacctmgr/cacctmgr.go (6)
350-409:ModifyAccountsuccess/error behaviour looks correct
ModifyAccountnow parses value lists with field‑specific messages, returnsErrorNetworkfor RPC failures, propagates backend rich errors, and returnsnilon success (both JSON and text paths). This is consistent and avoids the previous success‑as‑error pattern.
411-472:ModifyUsersuccess/error behaviour looks correct
ModifyUservalidates admin level, enforces single‑value fields, wraps parse errors asErrorCmdArg, and returnsnilon success (JSON and text). This matches the new error‑handling model and avoids logging JSON as an error.
508-552:FindAccount+PrintAccountListintegration looks good
FindAccountnow wraps argument/transport/backend errors asCraneErrorand delegates success rendering toPrintAccountList, which in turn returnsnilon success or empty results and propagates table printing errors. This matches the newerror-based API.
643-677:GetEventPluginConfigshould returnnilerror on success
GetEventPluginConfigreturnsutil.NewCraneErr(util.ErrorSuccess, "")on success instead ofnil. This breaks standard Go error handling—any caller usingif err != nilwill incorrectly treat success as failure:- return dbConf.Database, util.NewCraneErr(util.ErrorSuccess, "") + return dbConf.Database, nilVerify all call sites handle errors appropriately after this change.
200-232: Use idiomatic Go error handling: returnnilon success, notErrorSuccessCraneErrorFunctions like
AddAccount,AddUser,AddQos,DeleteAccount,DeleteUser, andDeleteQosshould returnnilon success instead ofutil.NewCraneErr(util.ErrorSuccess, ""). This aligns with Go conventions where the error return value should benilfor success and non-nilonly for actual failures. While the code currently works because callers special-caseCraneError, returning non-nil on success is non-idiomatic and creates confusion for new call sites or maintainers who follow standard Go error-checking patterns (if err != nil).Adopt the pattern already used in
ModifyAccountandModifyUser, which correctly returnnilon success, consistently across all add/delete/block/unblock operations.
474-506: Bug:ModifyQosreturns non‑nil error on success, breaking callers
ModifyQosreturnsutil.NewCraneErr(util.ErrorSuccess, "")on successful replies (both JSON and non‑JSON paths). Callers using the standard Go patternif err != nil { ... }will treat every successful QoS modification as a failure, causing commands to always exit with an error status.Return
nilon success and onlyCraneErroron real failures:
- Line 492-493: JSON success path should return
nil- Line 505-506: Non-JSON success path should return
nil- Keep error returns on failure unchanged
internal/cacctmgr/output.go (3)
178-232: Format error path inAccountFormatOutputis sound
AccountFormatOutputparsesFlagFormat, fills headers/data, and on unknown column returnsutil.ErrorInvalidFormatwith a clear message. On success it usesutil.FormatTableand returnsnil. The control flow and error code look good.
382-547: User/QoS format output functions validate format cleanly
UserFormatOutputandQosFormatOutputboth:
- Parse
FlagFormat, build aligned table headers/data.- Return
ErrorInvalidFormaton unknown column names.- Return
nilon success.This is clear and matches the rest of the error‑handling refactor.
307-332: Consider returningnilfor empty user/QoS lists to follow Go error conventions
PrintUserListandPrintQosListreturnutil.NewCraneErr(util.ErrorSuccess, "")for empty lists, but Go idiom treatsnilas the signal for success. Returning a non-nil error value—even with a success code—violates the standard error handling pattern whereerror == nilindicates success. For consistency and to align with Go conventions, returnnilinstead. Verify thatPrintAccountListand other similar functions in the codebase already follow this pattern.internal/cacctmgr/cmd.go (4)
155-276: Add/add‑user/add‑QoS command mapping and validation look correctThe add‑entity commands:
- Use
checkEmptyKVParamswhere appropriate (e.g.AddUserrequiresaccount).- Validate numeric QoS parameters via
validateUintValue.- Map unknown flags to
ErrorCmdArg.Apart from the already‑flagged global success‑as‑error pattern, these flows are logically sound.
278-351: Delete command dispatch and validation look correct
executeDelete*Commandfunctions:
- Require IDs (
name) and surface missing IDs asErrorCmdArg.- Parse KV flags and reject unknown ones.
- Delegate to
DeleteAccount/DeleteUser/DeleteQosfor actual work.No functional issues spotted here.
792-807: Show/reset command wiring and logger init match refactor goals
executeShow*andexecuteResetCommandsimply route to the corresponding high‑level functions (ShowAccounts,ShowUser,ShowQos,ShowTxn,ResetUserCredential) and propagate their errors, which fits the newerror‑centric API. Theinitfunction’sutil.InitCraneLogger()call standardizes logging for this package.Also applies to: 841-878, 880-906, 908-910
94-103: HardenParseCmdArgsagainst nil-pointer and non-CraneError errors
ParseCmdArgsassumes any non-nilresultfromexecuteCommandis a*util.CraneErrorand dereferences it unconditionally. In Go,errors.Ascan match a typed nil pointer—if the error's dynamic type is*util.CraneErrorbut the value is nil,errors.As(result, &craneError)returns true withcraneError == nil, causing a panic oncraneError.Message.Guard both the type check and nil pointer:
result := executeCommand(command) if result != nil { var craneError *util.CraneError - errors.As(result, &craneError) - if craneError.Message != "" { - log.Error(craneError.Message) - } - os.Exit(craneError.Code) + if errors.As(result, &craneError) && craneError != nil { + if craneError.Message != "" { + log.Error(craneError.Message) + } + os.Exit(craneError.Code) + } else { + log.Error(result.Error()) + os.Exit(int(util.ErrorGeneric)) + } } else { os.Exit(util.ErrorSuccess) }Also applies to: 112-139
internal/ccontrol/cmd.go (1)
459-485:checkEmptyKVParamsbehaviour is reasonable for this packageFor ccontrol,
checkEmptyKVParams:
- Rejects empty attribute sets (
"no attributes to modify").- Enforces required fields and reports which are missing.
This is a sensible validation layer for update/create commands; the logging behaviour is controlled through
CraneErroras intended.Also applies to: 487-489
internal/ccontrol/output.go (7)
145-160: Network error wrapping viaGrpcErrorSprintflooks good
getCranedNodesReplynow usesutil.NewCraneErr(util.ErrorNetwork, util.GrpcErrorSprintf(err, "Failed to show nodes")). This cleanly classifies transport failures as network errors and provides user‑friendly gRPC diagnostics.
161-198: Node output helpers refactor is straightforward
outputNodesandprintNodeDetailsnow just print to stdout and do not return errors. Formatting of state, resources and timestamps is unchanged; all error handling remains at the RPC/lookup layer. This is a reasonable separation.
272-362: Partition show path matches node pattern
getPartitionInfoReplyandShowPartitionsmirror the node logic: wrap gRPC failures asErrorNetwork, handle empty results viahandleEmptyPartitionResult, and print usingoutputPartitions. This is consistent with the rest of the refactor.
365-470: Reservation show path and formatting are coherent
getReservationInfoReplyclassifies network vs backend failures,ShowReservationsrespects JSON vs text, andoutputReservations+formatReservationDetailsproduce detailed but straightforward text output. No issues here.
508-519: Job output and missing-job hinting look good
outputJobsnow:
- Tracks printed job IDs and calls
checkMissingJobsto warn when requested IDs aren’t running.- Delegates to
printJobDetailswhich may returnCraneErroron user/group lookup issues.This is a nice UX improvement and keeps error handling localized.
Also applies to: 677-693
713-750: ShowLicenses network/backend error classification is correct
ShowLicenses:
- Uses
ErrorNetworkwithGrpcErrorSprintffor RPC failures.- Uses
ErrorBackendwhenreply.Okis false.- Treats empty lists differently depending on
queryAll.This aligns well with the new error‑handling scheme.
473-482: Critical:parseJobIdsreturns non‑nil "success" error whenqueryAllis true, breakingshow jobwith-aWhen
queryAllis true,parseJobIdsreturnsnil, util.NewCraneErr(util.ErrorSuccess, ""). IfShowJobstreats any non‑nilerrfromparseJobIdsas a failure, it will bail out before the RPC call, preventing "show all jobs" from working.Fix: Return
nil, nilinstead, allowing the RPC layer to interpret an empty filter list as "all jobs":func parseJobIds(jobIds string, queryAll bool) ([]uint32, error) { if queryAll { - return nil, util.NewCraneErr(util.ErrorSuccess, "") + return nil, nil } jobIdList, err := util.ParseJobIdList(jobIds, ",") if err != nil { return nil, util.WrapCraneErr(util.ErrorCmdArg, "Invalid job list specified: %s.", err) } return jobIdList, nil }Also applies to lines 694-711.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/ccontrol/ccontrol.go (1)
508-508: Remove trailing newline for consistency.The error message at line 508 contains a trailing newline (
\n), which is inconsistent with similar error messages elsewhere (e.g., lines 397-398, 480). This was flagged in a previous review and marked as addressed, but the trailing newline is still present.Apply this diff:
- return util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.\n", reply.GetReason())) + return util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.", reply.GetReason()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ccontrol/ccontrol.go(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/ccontrol/ccontrol.go (2)
internal/util/err.go (5)
NewCraneErr(61-66)ErrorBackend(39-39)ErrorGeneric(36-36)ErrorNetwork(38-38)ErrorSuccess(35-35)internal/util/grpc.go (1)
GrpcErrorSprintf(405-422)
🔇 Additional comments (1)
internal/ccontrol/ccontrol.go (1)
50-94: LGTM: Error aggregation pattern is consistent.The refactoring to build aggregated error messages for failed operations and return
CraneErrorinstances is well-implemented across all reply types.
Nativu5
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
internal/cacctmgr/cacctmgr.go (2)
239-242: Fix misleading error message for account deletion.Line 241 reports "Invalid user list specified" when parsing an account list fails. This is confusing and misleading.
Apply this diff:
accountList, err := util.ParseStringParamList(value, ",") if err != nil { - return util.WrapCraneErr(util.ErrorCmdArg, "Invalid user list specified: %v.\n", err) + return util.WrapCraneErr(util.ErrorCmdArg, "Invalid account list specified: %v.\n", err) }Based on past review comments.
318-321: Fix misleading error message for QoS deletion.Line 320 reports "Invalid user list specified" when parsing a QoS list fails. This is confusing and misleading.
Apply this diff:
qosList, err := util.ParseStringParamList(value, ",") if err != nil { - return util.WrapCraneErr(util.ErrorCmdArg, "Invalid user list specified: %v.\n", err) + return util.WrapCraneErr(util.ErrorCmdArg, "Invalid QoS list specified: %v.\n", err) }Based on past review comments.
internal/ccontrol/cmd.go (4)
452-456: Preserve original error code from DeleteReservation.Line 454 wraps the error from
DeleteReservationwithErrorGeneric, butDeleteReservationreturnsutil.CraneErrorwithErrorNetwork(line 491 in ccontrol.go) orErrorBackend(lines 500, 508 in ccontrol.go).Apply this diff:
err := DeleteReservation(name) if err != nil { - return util.WrapCraneErr(util.ErrorGeneric, "%s\n", err) + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return craneErr + } + return util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("%s\n", err)) } return nil
329-336: Preserve original error codes from ModifyPartitionAcl.Lines 330 and 335 wrap errors from
ModifyPartitionAclwithErrorGeneric, butModifyPartitionAclreturnsutil.CraneErrorwith specific codes likeErrorNetwork(line 383 in ccontrol.go) orErrorBackend(lines 392, 397 in ccontrol.go). Wrapping loses this granularity.Apply this pattern to both error handling branches:
if err := ModifyPartitionAcl(FlagPartitionName, true, FlagAllowedAccounts); err != nil { - return util.WrapCraneErr(util.ErrorGeneric, "%s\n", err) + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return craneErr + } + return util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("%s\n", err)) }Apply the same fix to line 335.
239-243: Preserve original error code from ChangeNodeState.Line 241 wraps the error from
ChangeNodeStatewithErrorCmdArg, butChangeNodeStatereturnsutil.CraneErrorwith codes likeErrorNetwork(line 354 in ccontrol.go) orErrorBackend(line 363 in ccontrol.go). Wrapping withErrorCmdArgloses this information and misclassifies the error type.Apply this diff to preserve the original error code:
err := ChangeNodeState(FlagNodeName, FlagState, FlagReason) if err != nil { - return util.WrapCraneErr(util.ErrorCmdArg, "change node state failed: %s\n", err) + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return craneErr + } + return util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("change node state failed: %s\n", err)) } return nil
429-433: Preserve original error code from CreateReservation.Line 431 wraps the error from
CreateReservationwithErrorGeneric, butCreateReservationreturnsutil.CraneErrorwith specific codes likeErrorCmdArg(lines 408, 412, 426, etc. in ccontrol.go),ErrorNetwork(line 463 in ccontrol.go), orErrorBackend(lines 472, 480 in ccontrol.go).Apply this diff:
err := CreateReservation() if err != nil { - return util.WrapCraneErr(util.ErrorGeneric, "%s\n", err) + var craneErr *util.CraneError + if errors.As(err, &craneErr) { + return craneErr + } + return util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("%s\n", err)) } return nilinternal/ccontrol/ccontrol.go (1)
508-508: Remove trailing newline from error message.Line 508 has a trailing
\nin the error message, which is inconsistent with similar error messages in this file (e.g., lines 398, 480) and can cause extra blank lines in output.Apply this diff:
- return util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.\n", reply.GetReason())) + return util.NewCraneErr(util.ErrorBackend, fmt.Sprintf("Failed to delete reservation: %s.", reply.GetReason()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/cacctmgr/cacctmgr.go(19 hunks)internal/cacctmgr/cmd.go(34 hunks)internal/cacctmgr/output.go(16 hunks)internal/ccontrol/ccontrol.go(17 hunks)internal/ccontrol/cmd.go(11 hunks)internal/ccontrol/output.go(13 hunks)internal/util/err.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-12T03:17:51.694Z
Learnt from: 1daidai1
Repo: PKUHPC/CraneSched-FrontEnd PR: 285
File: internal/cinfo/cinfo.go:363-364
Timestamp: 2025-06-12T03:17:51.694Z
Learning: 在 cinfo 命令中，使用 -o/--format 自定义输出时，应省略节点数为 0 的分区，不予显示。
Applied to files:
internal/ccontrol/output.go
🧬 Code graph analysis (6)
internal/ccontrol/ccontrol.go (5)
internal/util/err.go (6)
NewCraneErr(63-68)ErrorBackend(41-41)WrapCraneErr(70-76)ErrorCmdArg(39-39)NewCraneErrFromGrpc(78-99)ErrorNetwork(40-40)internal/cacctmgr/cmd.go (1)
FlagJson(64-64)internal/ccontrol/cmd.go (1)
FlagJson(47-47)internal/cbatch/cmd.go (1)
FlagJson(63-63)internal/cqueue/cmd.go (1)
FlagJson(45-45)
internal/ccontrol/cmd.go (4)
internal/ccontrol/parser.go (1)
CControlCommand(33-35)internal/util/err.go (6)
CraneError(46-50)ErrorSuccess(37-37)NewCraneErr(63-68)ErrorCmdArg(39-39)WrapCraneErr(70-76)ErrorGeneric(38-38)internal/ccontrol/ccontrol.go (1)
ChangeNodeState(316-368)internal/util/log.go (1)
InitCraneLogger(35-37)
internal/cacctmgr/cmd.go (3)
internal/util/err.go (5)
CraneError(46-50)ErrorSuccess(37-37)NewCraneErr(63-68)ErrorCmdArg(39-39)WrapCraneErr(70-76)internal/cacctmgr/cacctmgr.go (9)
AddAccount(93-141)AddUser(143-198)AddQos(200-232)DeleteUser(272-311)BlockAccountOrUser(553-596)UnblockAccountOrUser(598-641)ModifyAccount(350-409)ModifyUser(411-472)ModifyQos(474-506)internal/cacctmgr/output.go (1)
ShowTxn(89-141)
internal/cacctmgr/cacctmgr.go (2)
internal/util/string.go (3)
CheckEntityName(609-629)ParseStringParamList(1132-1141)GetValidNodeList(1169-1195)internal/util/err.go (4)
WrapCraneErr(70-76)NewCraneErr(63-68)ErrMsg(233-238)NewCraneErrFromGrpc(78-99)
internal/cacctmgr/output.go (3)
internal/util/err.go (3)
NewCraneErr(63-68)WrapCraneErr(70-76)ErrMsg(233-238)internal/util/formatter.go (1)
FmtJson(58-58)internal/util/table.go (2)
FormatTable(51-96)SetBorderTable(41-49)
internal/ccontrol/output.go (1)
internal/util/err.go (6)
WrapCraneErr(70-76)ErrorCmdArg(39-39)NewCraneErrFromGrpc(78-99)ErrorNetwork(40-40)NewCraneErr(63-68)ErrorBackend(41-41)
🔇 Additional comments (10)
internal/util/err.go (3)
78-99: LGTM! Well-structured gRPC error handling.The
NewCraneErrFromGrpcfunction provides clear, user-friendly messages for common gRPC error conditions while maintaining the underlying error as the cause. The switch-case mapping for specific gRPC codes is appropriate and helpful.
118-120: LGTM! Consistent error silencing behavior.Silencing both usage and errors when CraneError code is not
ErrorCmdArgmakes sense—only argument errors should trigger usage display.
137-139: LGTM! Appropriate error logging before exit.Logging the CraneError message when non-empty and non-ErrorCmdArg provides visibility while avoiding duplicate logging for argument validation failures.
internal/cacctmgr/cmd.go (2)
908-910: LGTM! Proper logger initialization.The
init()function correctly initializes the Crane logger, aligning with the pattern used across other command packages in this PR.
127-139: LGTM! Correct CraneError-based exit handling.The ParseCmdArgs function properly:
- Extracts CraneError using
errors.As- Logs the message when non-empty
- Exits with the appropriate error code
This aligns with the new CraneError-centric error handling pattern.
internal/ccontrol/cmd.go (1)
487-489: LGTM! Proper logger initialization.The
init()function correctly initializes the Crane logger, consistent with other packages in this PR.internal/ccontrol/output.go (1)
256-270: LGTM! Clean error handling in ShowNodes.The function properly:
- Returns error from
getCranedNodesReply- Handles empty results appropriately
- Calls void
outputNodeson success- Returns nil on success
This pattern is clean and consistent with Go conventions.
internal/cacctmgr/output.go (2)
125-131: LGTM! Correct JSON handling pattern.The function properly prints JSON output to stdout and returns nil on success (line 128), which is consistent with the pattern used in other Show* functions (e.g., ShowAccounts at lines 287-288). This avoids logging JSON as an error message.
144-175: LGTM! PrintAccountList correctly returns nil on empty list.Returning
nilfor an empty list (line 146) is correct and follows Go conventions where nil means success.internal/cacctmgr/cacctmgr.go (1)
93-141: LGTM! AddAccount correctly returns nil on success.The function properly returns
nilon success (lines 137, 129) rather than a success-coded error, following Go conventions.
| err := HoldReleaseJobs(jobIds, true) | ||
| if err != nil { | ||
| log.Errorf("hold jobs failed: %s", err) | ||
| return util.ErrorGeneric | ||
| return util.WrapCraneErr(util.ErrorGeneric, "hold jobs failed: %s\n", err) | ||
| } | ||
| return util.ErrorSuccess | ||
| return nil |
There was a problem hiding this comment.
Preserve original error codes from HoldReleaseJobs.
Lines 364 and 377 wrap errors from HoldReleaseJobs with ErrorGeneric, losing the original CraneError codes like ErrorCmdArg (lines 99, 136, 152, 156 in ccontrol.go) or ErrorNetwork (lines 117, 167 in ccontrol.go).
Apply this pattern to both locations:
err := HoldReleaseJobs(jobIds, true)
if err != nil {
- return util.WrapCraneErr(util.ErrorGeneric, "hold jobs failed: %s\n", err)
+ var craneErr *util.CraneError
+ if errors.As(err, &craneErr) {
+ return craneErr
+ }
+ return util.NewCraneErr(util.ErrorGeneric, fmt.Sprintf("hold jobs failed: %s\n", err))
}
return nilApply the same fix to lines 375-379 for executeReleaseCommand.
Also applies to: 375-379
🤖 Prompt for AI Agents
In internal/ccontrol/cmd.go around lines 362-366 and 375-379, the code currently
wraps errors from HoldReleaseJobs using util.ErrorGeneric which discards the
original CraneError code; modify both places to detect whether err is already a
CraneError (or exposes the original error code), and when it is call
util.WrapCraneErr with that original code (rather than util.ErrorGeneric) and
the same message; fall back to util.ErrorGeneric only if the error is not a
CraneError. Ensure both hold and release handlers use the same pattern so
original ErrorCmdArg/ErrorNetwork codes are preserved.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.