fix: duplicate modification message output #367
Conversation
WalkthroughRefactors account, user, and QoS modify flows to accept slices of ModifyParam, build per-parameter ModifyFieldOperation entries, and send batched RPC requests; updates protobufs to add ModifyFieldOperation and replace single-field modify_* request fields with repeated operations. Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Handler
participant Cmd as Param Collector
participant CAcct as cacctmgr.Modify*
participant RPC as RPC Stub (CraneCtld)
rect rgb(230, 245, 230)
CLI->>Cmd: parse flags -> create ModifyParam(s)
Cmd-->>CLI: params []ModifyParam
CLI->>CAcct: ModifyAccount/ModifyUser/ModifyQos(params, ...)
end
rect rgb(230, 240, 255)
CAcct->>CAcct: for each param: validate & parse value
CAcct->>CAcct: build ModifyFieldOperation list
CAcct->>RPC: stub.Modify*(Request{name, operations: [...]})
RPC-->>CAcct: Reply (ok / rich_error_list)
CAcct-->>CLI: exit code / output (success or rich errors)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (4)
internal/cacctmgr/cmd.go (3)
587-676: Account modify aggregation correctly centralizes operationsCollecting all
set/add/deleteclauses intoparams []ModifyParamand issuing a singleModifyAccountcall aligns with the new proto and removes the prior risk of duplicate success/failure messages. Logic per field and per operation type looks consistent with existing semantics.You could later drop the intermediate
Flag*assignments here (e.g.,FlagDescription,FlagSetPartitionList) and write values directly intoModifyParamto reduce global state churn, but that’s not required for this fix.
724-813: User modify aggregation is consistent and validates admin level earlyThe new
params []ModifyParamflow for user modifications mirrors the account path, and the explicit admin-level validation before building operations is good. This should ensure only a singleModifyUserRPC (and result message) per command.Similar to the account path, the
Flag*intermediates (e.g.,FlagSetPartitionList,FlagUserDefaultQos) could be made local or inlined intoModifyParamlater to reduce reliance on package globals.
850-901: QoS modify path: consider settingRequestTypeexplicitly and using itCurrently, QoS
ModifyParamentries are created withoutRequestType, andModifyQos(ininternal/cacctmgr/cacctmgr.go) ignoresparam.RequestTypeand relies on the proto’s defaultOperationType. That’s probably fine today since onlysetsemantics are supported, but it’s less explicit and could bite you ifOperationTypeever changes or QoS gains add/delete semantics.I’d recommend:
- Explicitly setting
RequestType: protos.OperationType_Overwritewhen building QoSModifyParamvalues; and- In
ModifyQos, passingType: param.RequestTypewhen appendingModifyFieldOperation.Example diff for this file:
- case "maxcpusperuser": + case "maxcpusperuser": if err := validateUintValue(value, "maxCpusPerUser", 32); err != nil { return util.ErrorCmdArg } FlagMaxCpu = value params = append(params, ModifyParam{ ModifyField: protos.ModifyField_MaxCpusPerUser, NewValue: FlagMaxCpu, + RequestType: protos.OperationType_Overwrite, })(and similarly for the other QoS fields).
Please double‑check the
OperationTypeenum definition (in your proto) to ensure the current default actually corresponds to overwrite; if not, this change becomes functionally necessary rather than cosmetic.internal/cacctmgr/cacctmgr.go (1)
785-822: QoS modification batching is correct but should ideally carry explicit operation types
ModifyQoscorrectly aggregates QoS field updates intooperationsand issues a single RPC, fixing duplicate messages along this path as well. However, it currently ignoresparam.RequestTypeand relies on the defaultOperationTypeon the server side.To stay consistent with account/user handling and avoid relying on enum defaults, consider:
for _, param := range params { - req.Operations = append(req.Operations, &protos.ModifyFieldOperation{ - ModifyField: param.ModifyField, - ValueList: []string{param.NewValue}, - }) + req.Operations = append(req.Operations, &protos.ModifyFieldOperation{ + ModifyField: param.ModifyField, + ValueList: []string{param.NewValue}, + Type: param.RequestType, + }) }This pairs with explicitly setting
RequestTypeinexecuteModifyQosCommand(see comment ininternal/cacctmgr/cmd.go), and keeps behavior robust if QoS ever supports non‑overwrite operations.Please confirm the current server expects or ignores
OperationTypefor QoS modifications so we know whether this is just a clarity improvement or required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cacctmgr/cacctmgr.go(3 hunks)internal/cacctmgr/cmd.go(8 hunks)protos/Crane.proto(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cacctmgr/cmd.go (1)
internal/cacctmgr/cacctmgr.go (3)
ModifyAccount(646-712)ModifyUser(714-783)ModifyQos(785-822)
internal/cacctmgr/cacctmgr.go (3)
internal/cacctmgr/cmd.go (2)
ModifyParam(92-96)FlagForce(60-60)internal/util/err.go (2)
ExitCode(30-30)ErrorCmdArg(36-36)internal/util/string.go (1)
ParseStringParamList(1087-1096)
🔇 Additional comments (6)
internal/cacctmgr/cmd.go (1)
92-96: ModifyParam struct shape looks appropriateFields map cleanly to
ModifyFieldOperationusage and keep the command layer decoupled from proto details. No issues here.protos/Crane.proto (3)
343-347:ModifyFieldOperationmessage design is compatible with the new param modelThe
modify_field,value_list, andtypefields map cleanly to the GoModifyParamusage and allow batching heterogeneous modifications in a single request.
349-379: Modify*Request message reshaping aligns with batched operationsUsing
nameplusrepeated ModifyFieldOperation operationsinModifyAccountRequest,ModifyUserRequest, andModifyQosRequestmatches the new client‑side aggregation and enables multi‑field changes per RPC. Field numbering (keepinguid = 1and introducing new tags for the additional fields) looks safe for coordinated client/server upgrades.
990-1129: Service definition changes are formatting-onlyThe changes in the
serviceblocks here are line‑wrapping and comment formatting; RPC names and request/reply types are unchanged, so there’s no behavioral impact.internal/cacctmgr/cacctmgr.go (2)
646-712: Account modification implementation matches new proto and avoids duplicate output
ModifyAccountnow iterates overparams, parses value lists once per param, validates single-value fields (Description, DefaultQos), and builds a singleModifyAccountRequest.Operationsslice before calling the RPC. Error handling is consistent, and success/failure messages are printed exactly once per command, addressing the duplicate‑message issue.
714-783: User modification batching is correct and preserves validation semantics
ModifyUsercorrectly validatesAdminLevelupfront, enforces single‑value semantics forAdminLevel,DefaultQos, andDefaultAccount, and appends each operation (including add/delete) with itsRequestType. As with accounts, this yields one RPC and one set of messages per command instead of per‑field calls.
|
rebase 一下吧 |
edragain2nd
force-pushed
the
fix/cacctmgr_modify
branch
from
aacc673 to
99e200b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cacctmgr/cacctmgr.go (1)
362-399:ModifyAccount: validation and batched operations look correct
- Per‑param parsing via
ParseStringParamListand tailored error messages forQos/Partitionare reasonable.- Enforcing single values for
DefaultQosandDescriptionavoids ambiguous multi‑value updates.- Building
req.OperationswithModifyField,ValueList, andType: param.RequestTypematches the new proto and enables multiple field updates in a single RPC.- Consolidated reply handling (single success/failure message with iteration over
RichErrorList) prevents duplicated output and improves clarity.Only minor style nit:
valueList/errcould be declared inside the loop to reduce scope, but current code is functionally fine.Also applies to: 415-428
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cacctmgr/cacctmgr.go(6 hunks)internal/cacctmgr/cmd.go(8 hunks)protos/Crane.proto(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cacctmgr/cmd.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cacctmgr/cacctmgr.go (3)
internal/cacctmgr/cmd.go (2)
ModifyParam(102-106)FlagForce(60-60)internal/util/err.go (4)
ExitCode(30-30)ErrorCmdArg(36-36)ErrorSuccess(34-34)ErrMsg(203-208)internal/util/string.go (1)
ParseStringParamList(1132-1141)
🔇 Additional comments (4)
protos/Crane.proto (3)
352-356: NewModifyFieldOperationmessage is consistent with client usageThe fields (
ModifyField,value_list,OperationType) line up with howinternal/cacctmgr/cacctmgr.goconstructs operations; using a shared operation message across account/user/QoS requests looks clean and extensible.
358-363: Batched modify request/reply shapes look coherentSwitching
ModifyAccountRequest,ModifyUserRequest, andModifyQosRequestto userepeated ModifyFieldOperation operationsplus changingModifyQosReplytorepeated RichError rich_error_listmatches the new client logic (building multiple operations per call and iterating over rich errors). From the frontend side the shapes are internally consistent and enable clearer multi-error reporting.Also applies to: 370-377, 384-388, 390-393
1005-1010: RPC formatting changes are cosmetic onlyThe reflowed
rpcdeclarations and wrapped comments (multi-linereturns (...)and banner comments) are purely stylistic and improve readability without changing wire contracts or method names.Also applies to: 1029-1033, 1042-1045, 1051-1058, 1062-1063, 1069-1070, 1079-1081, 1090-1092, 1095-1098, 1103-1106, 1108-1113, 1120-1125, 1131-1138
internal/cacctmgr/cacctmgr.go (1)
431-470:ModifyUser: good per‑field validation and operation construction
- Admin level values are validated early against the allowed set (
none|operator|admin), which prevents bad requests.ParseStringParamListfollowed by a single‑value check forAdminLevel,DefaultQos, andDefaultAccountcorrectly enforces scalar semantics where expected.req.Operationsis populated withModifyField, parsedValueList, andType: param.RequestType, consistent with the new proto.- Reply handling mirrors
ModifyAccount, with a single success/failure message and detailed iteration overRichErrorList, avoiding repeated messages.No functional issues spotted; this aligns with the new batched modify design.
Also applies to: 487-500
|
请 @huerni 帮忙看一下 cacctmgr 前后端的修改吧？ |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.