shaipan commented on PR #22840: URL: https://github.com/apache/camel/pull/22840#issuecomment-4351646531
> Thanks for improving the error resilience in the Salesforce Streaming subscription handling, @shaipan! This addresses a real operational pain point. A few observations: > > **Missing tests**: Per [project standards](https://github.com/apache/camel/blob/main/CLAUDE.md), every PR should include tests for new behavior. The new error handling paths in `subscriptionFailed()` and the extended `isTemporaryError()` method are not covered by the existing `SubscriptionHelperTest`. Unit tests verifying that `IOException`, `TransportException`, and `TimeoutException` are treated as temporary, and that the various 401/403 error strings trigger retry, would strengthen confidence in these changes. > > **401/403 retry without token refresh**: Treating `AUTHENTICATION_INVALID` (401) and authorization errors (403) as retryable is reasonable for transient failures. However, the retry path re-calls `subscribe()` without explicitly triggering a session token refresh. If the token is genuinely expired, all retries may fail identically, consuming the full backoff budget before aborting. Worth confirming that the CometD handshake/subscribe path handles token refresh internally. > > **Minor**: Two comments explaining the non-obvious `Tasks.foregroundTask()` usage (using `initialDelay` as a sleep and `maxIterations(1)` to run once) were removed. The pattern isn't self-documenting — consider keeping them. > > _Claude Code on behalf of Guillaume Nodet_ @gnodet The methods are private and the existing test pattern in the file doesn't cover listener callbacks -- 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]
