gnodet commented on code in PR #22840:
URL: https://github.com/apache/camel/pull/22840#discussion_r3166656563
##########
components/camel-salesforce/camel-salesforce-component/src/main/java/org/apache/camel/component/salesforce/internal/streaming/SubscriptionHelper.java:
##########
@@ -255,6 +261,9 @@ private void subscriptionFailed(StreamingApiConsumer
firstConsumer, Message mess
for (var consumer : consumers) {
subscribe(consumer);
}
+ } else if (error.matches(CHANNEL_INVALID_PATTERN)) {
+ LOG.warn("Channel invalid for channel {}, removing from
subscription list", channelName);
+ channelsToSubscribe.remove(channelName);
Review Comment:
In this branch, `abort` stays `true`, so the code falls through to:
```java
if (abort && client != null) {
consumer.getExceptionHandler().handleException(...);
}
```
Meaning the channel is removed from `channelsToSubscribe` **and** all
consumers for that channel receive an exception. Is that the intended behavior?
If so, it makes sense (clean up + notify), but setting `abort = false` after
the removal would be the alternative if the intent is to silently drop the bad
channel.
##########
components/camel-salesforce/camel-salesforce-component/src/main/java/org/apache/camel/component/salesforce/internal/streaming/SubscriptionHelper.java:
##########
@@ -219,7 +225,9 @@ private void subscriptionFailed(StreamingApiConsumer
firstConsumer, Message mess
boolean abort = true;
LOG.warn(msg);
- if (isTemporaryError(message)) {
+ if (isTemporaryError(message) || error.equals(AUTHENTICATION_INVALID)
|| error.startsWith(DENIED_BY_SEC_POLICY)
+ || error.startsWith(AUTHORIZATION_ERROR) ||
error.equals(ORG_LIMIT_ERROR)
Review Comment:
Nit: `AUTHORIZATION_ERROR` is `"403::"` and `ORG_LIMIT_ERROR` is
`"403::Organization total events daily limit exceeded"`. Since
`ORG_LIMIT_ERROR` starts with `"403::"`, the `startsWith(AUTHORIZATION_ERROR)`
check already covers it — the `equals(ORG_LIMIT_ERROR)` is redundant.
```suggestion
|| error.startsWith(AUTHORIZATION_ERROR)
```
(Note: `DENIED_BY_SEC_POLICY` uses a single colon
`"403:denied_by_security_policy"` so its separate `startsWith` check is
correctly needed.)
##########
components/camel-salesforce/camel-salesforce-component/src/main/java/org/apache/camel/component/salesforce/internal/streaming/SubscriptionHelper.java:
##########
@@ -219,7 +225,9 @@ private void subscriptionFailed(StreamingApiConsumer
firstConsumer, Message mess
boolean abort = true;
LOG.warn(msg);
- if (isTemporaryError(message)) {
+ if (isTemporaryError(message) || error.equals(AUTHENTICATION_INVALID)
|| error.startsWith(DENIED_BY_SEC_POLICY)
+ || error.startsWith(AUTHORIZATION_ERROR) ||
error.equals(ORG_LIMIT_ERROR)
+ || error.equals("Missing error message")) {
Review Comment:
This matches the synthetic default set earlier when `error == null`:
```java
if (error == null) {
error = "Missing error message";
}
```
Previously, a message with **no error field at all** would abort the
subscription. Now it silently retries with backoff. This is a significant
behavioral change — unknown/unclassified failures become retryable instead of
surfacing to the consumer's exception handler. If there's genuinely no error
information, retrying blindly (up to `maxBackoff`) may mask real problems.
Was this intentional? If so, it might be worth a comment explaining the
rationale.
--
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]
