Copilot commented on code in PR #853:
URL:
https://github.com/apache/skywalking-banyandb/pull/853#discussion_r2544327328
##########
banyand/trace/svc_liaison.go:
##########
@@ -175,13 +177,21 @@ func (l *liaison) PreRun(ctx context.Context) error {
// Calculate max handoff size based on percentage of disk space
// Formula: totalDisk * maxDiskUsagePercent *
handoffMaxSizePercent / 10000
// Example: 100GB disk, 95% max usage, 10% handoff = 100 * 95 *
10 / 10000 = 9.5GB
- maxSize := 0
+ var maxSizeBytes uint64
if l.handoffMaxSizePercent > 0 {
totalSpace := l.lfs.MustGetTotalSpace(l.dataPath)
// Divide after each multiplication to avoid overflow
with large disk capacities
- maxSizeBytes := totalSpace *
uint64(l.maxDiskUsagePercent) / 100 * uint64(l.handoffMaxSizePercent) / 100
- maxSize = int(maxSizeBytes / 1024 / 1024)
+ maxSizeBytes = totalSpace *
uint64(l.maxDiskUsagePercent) / 100 * uint64(l.handoffMaxSizePercent) / 100
+ }
+ if maxSizeBytes == 0 {
+ return fmt.Errorf("handoff max size is 0 because
handoff-max-size-percent is 0 or not set. " +
+ "Set BYDB_HANDOFF_MAX_SIZE_PERCENT environment
variable or --handoff-max-size-percent flag to enable handoff storage limit")
}
Review Comment:
The logic for handling `handoffMaxSizePercent = 0` is inconsistent. When
`handoffMaxSizePercent` is 0:
1. The validation at line 121 allows it (valid range is 0-100)
2. The calculation at line 181 is skipped, leaving `maxSizeBytes = 0`
3. This error is then triggered at line 186
This creates a confusing user experience. If data nodes are configured but
`handoffMaxSizePercent` is 0, users get an error instead of handoff being
disabled.
Consider one of these approaches:
1. **Recommended**: Add `l.handoffMaxSizePercent > 0` to the condition at
line 176, so handoff is only initialized when explicitly configured with a
non-zero percentage (consistent with the failed-parts pattern at lines 160-172)
2. Alternative: If 0 means "unlimited", remove this error check and update
the flag description to clarify this behavior
Also note: The error message incorrectly states the only cause is
`handoff-max-size-percent` being 0, but `maxSizeBytes` could also be 0 if
`totalSpace` is very small or `maxDiskUsagePercent` is 0.
--
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]
