Aias00 commented on PR #6529:
URL: https://github.com/apache/shenyu/pull/6529#issuecomment-5156775458
Reviewed #6529 — fixes #6464. The root-cause analysis is correct and the fix
is sound: `Constants.HTTP_RETRY_BACK_OFF_SPEC` was `"default"`, used as the
exchange attribute *key*, so nothing ever wrote under it and
`AbstractHttpClientPlugin` always fell back to `DefaultRetryStrategy`, leaving
`fixed`/`exponential`/`custom` unreachable. Renaming the key to
`"httpRetryBackOffSpec"`, adding `DivideRuleHandle.retryBackOffSpec`,
populating it in `DividePlugin`, switching the `switch` to
`HttpRetryBackoffSpecEnum`, and replacing `CustomRetryStrategy`'s `return null`
with an explicit `Mono.error` all address the reported defects. Backward
compatibility is fine: existing rule-handle JSON lacks the new field, Gson
leaves it at the field initializer (`"default"`), so existing configs keep
routing to `DefaultRetryStrategy`. DB IDs are collision-free and consistent
across all 6 init dialects and 5 upgrade scripts. The constant-value change
only affects an in-memory attribute key (no persiste
d JSON key changes; only 3 Java consumers).
Non-blocking, but worth addressing before merge:
1. (should_fix) No regression test for the actual bug.
`DivideRuleHandleTest.testGetterSetter` covers every sibling field except
`retryBackOffSpec`; `DividePluginTest` doesn't assert
`Constants.HTTP_RETRY_BACK_OFF_SPEC` is placed into the exchange from
`ruleHandle.getRetryBackOffSpec()`; `RetryStrategyTest` exercises strategies in
isolation but not the selection switch in `AbstractHttpClientPlugin.execute`.
The whole point of the PR — that a configured non-default spec is actually
selected — is untested, so #6464 can regress silently. Suggested additions: (a)
a `DividePluginTest` case asserting the attribute is set from the rule handle
(mirroring the `HTTP_RETRY`/`RETRY_STRATEGY` puts); (b)
`HttpRetryBackoffSpecEnumTest` covering `acquireByName` for each name + null +
unknown → `DEFAULT_BACKOFF` (every other enum in the package has one); (c)
extend `testGetterSetter` to set/assert `retryBackOffSpec`.
2. (nit) The new `retryBackOffSpec` `plugin_handle` row uses `sort=0`,
identical to `retryStrategy`, so the two fields' admin-UI order is
non-deterministic. Give it a distinct sort (e.g. 1).
3. (nit / open question) `CustomRetryStrategy` now throws
`UnsupportedOperationException("Please implement CustomRetryStrategy via
SPI")`. Returning an explicit error is a clear improvement over `return null`,
but I couldn't find an SPI/registry for this strategy in the diff or under
`shenyu-plugin-httpclient`. If no extension point exists, consider hiding
`custom` from the `RETRY_BACKOFF_SPEC` dict until one does, since selecting it
today always yields a 503.
Note: PR is in `BEHIND` merge state (needs a rebase onto master before
merge).
--
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]
