Aias00 commented on PR #6895:
URL: https://github.com/apache/shenyu/pull/6895#issuecomment-5193351868
Thanks for this fix — the root cause (matching Spring's `@Service` instead
of Dubbo's) is correctly addressed, and the new `getRpcExt` mirrors
`DubboServiceProcessor`. A few observations from reading the diff and
surrounding code.
**`rpcExt` builder diverges from
`ApacheDubboServiceBeanListener.buildRpcExt`**
(`ApacheDubboServiceBeanListener.java:225-252`). The listener's builder sets
`.serialization(serviceBean.getSerialization())` (L236) and populates
per-method `DubboRpcMethodExt` (L239-250); the new `getRpcExt` in
`ServiceProcessor` (and the existing `DubboServiceProcessor:86-99`) sets
neither. For old-style `@Service` users this means the registered `rpcExt`
omits `serialization` and per-method loadbalance/retries/timeout/sent. If
downstream (`AbstractDubboMetaDataHandler` / `ApacheDubboConfigCache`) selects
the serializer from `rpcExt.serialization`, these users silently fall back to
the default. Could you confirm whether the omission is intentional, or extract
a shared `DubboRpcExtBuilders.from(ServiceBean)` helper so all three builders
stay aligned?
**No test covers the fix path.** The PR body's "submit test cases" checkbox
is unchecked and no test files changed. The highest-value missing test: a
`ServiceProcessorTest` proving (a) a Dubbo `@Service`-annotated bean whose
instance is a `ServiceBean` gets `beanPath = annotation.path()` and a populated
`rpcExt`, and (b) a non-`ServiceBean` instance gets the `"{}"` fallback. That
guards the exact regression this PR fixes.
Minor: `serviceBean.getProtocol().getName()` (new `getRpcExt`) can NPE if
`getProtocol()` is null — same pre-existing pattern as
`DubboServiceProcessor:88` / `ApacheDubboServiceBeanListener:227`, so not a
regression, just noting the line is reachable with an unconfigured
`ServiceBean`.
--
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]
