Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| func getTasksEnabledFromEnv() bool { | ||
| func getTasksDisabledFromEnv() bool { |
There was a problem hiding this comment.
🟡 Suggestion: getTasksDisabledFromEnv actually returns whether GetTasks is enabled: unset returns true, BATON_GET_TASKS=false returns false, and the result goes into getTasksEnabled. A reader who trusts the name could write getTasksEnabled: !getTasksDisabledFromEnv() and silently flip the behavior. Consider keeping the old name getTasksEnabledFromEnv with only the default changed. (confidence: high)
| serviceClient: serviceClient, | ||
| taskQueue: newTaskQueue(taskConcurrency), | ||
| getTasksEnabled: getTasksEnabledFromEnv(), | ||
| getTasksEnabled: getTasksDisabledFromEnv(), |
There was a problem hiding this comment.
🟡 Suggestion: Every service-mode connector now uses GetTasks by default, and there's no fallback to GetTask. TestNextReturnsGetTasksErrorWhenEnabled confirms that Unimplemented comes back as an error, and connectorrunner/runner.go:234-241 then just backs off and retries forever. A connector pointed at a C1 endpoint or proxy that doesn't serve GetTasks would stop picking up tasks and only log errors. Consider falling back to GetTask on codes.Unimplemented, or at least documenting BATON_GET_TASKS=false as the opt-out in the PR/release notes. (confidence: medium)
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
We added this a while back but never made it the default. This improves performance significantly for connectors that do a lot of grants/revokes, and for event feeds.
2f63d27 to
476bfa6
Compare
| raw, ok := os.LookupEnv(getTasksEnv) | ||
| if !ok { | ||
| return false | ||
| return true |
There was a problem hiding this comment.
🟡 Suggestion (prior, still present; confidence: medium): GetTasks is now on by default for every service-mode connector, and there's no fallback to GetTask when the server returns codes.Unimplemented. Next returns the error (manager.go:276-279) and runner.go:234-241 just backs off and retries forever, so against an endpoint without GetTasks the connector stops processing tasks. You could fall back by setting c.getTasksEnabled = false on Unimplemented, or at least document BATON_GET_TASKS=false as the opt-out.
| enabled, err := strconv.ParseBool(raw) | ||
| if err != nil { | ||
| return false | ||
| return true |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): Unparseable values now fall back to enabled. So an operator who tries to opt out with BATON_GET_TASKS=no or =off (which strconv.ParseBool rejects) silently gets GetTasks. Log a warning on the parse error, or default to false when the variable is set but invalid, since setting it at all signals intent to change the default.
Superseded — see the current review report for commit
|
General PR Review: Default to using the get tasks API.Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe PR flips I applied the repo-local criteria:
Security IssuesNone found. Correctness IssuesNone found. Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
1 participant
We added this a while back but never made it the default. This improves performance significantly for service mode connectors that do a lot of grants/revokes, and for service mode connectors that have event feeds.