wy471x commented on PR #6456: URL: https://github.com/apache/shenyu/pull/6456#issuecomment-5165537007
> Follow-up after the new commits ([d05fec8](https://github.com/apache/shenyu/commit/d05fec8182f31c1131831c520572aa9abee00216) + [f0a26bf](https://github.com/apache/shenyu/commit/f0a26bf6d6621f2a8885fb0df22c2711133efe71)). Thanks — the substantive issues are addressed. Per-finding: > > * **#1 blocker (null-url NPE in `nativeCreateOrUpdate` + CI red): fixed.** `IpUtils.parseHostPort` and `CommonUpstreamUtils.normalizeUrl` now throw `IllegalArgumentException("Invalid upstream URL, null or blank")` on blank/null, and `testNativeCreate` now sets a URL. The originally-red `k8s-examples-http` is passing again. Verified in commit [f0a26bf](https://github.com/apache/shenyu/commit/f0a26bf6d6621f2a8885fb0df22c2711133efe71). > * **#2 listener swallowed exceptions inside `@Transactional`: fixed.** `handleUpdated`/`handleDeleted` now `throw e` after logging (commit [d05fec8](https://github.com/apache/shenyu/commit/d05fec8182f31c1131831c520572aa9abee00216)), so `onChange` rolls back on failure — atomicity restored. > * **#3 service write paths lacked host:port fallback: fixed.** `deleteBySelectorIdAndUrl` and `changeStatusBySelectorIdAndUrl` now check `effect == 0` and fall back to `matchByHostAndPort` + delete/update by id, mirroring the listener path ([d05fec8](https://github.com/apache/shenyu/commit/d05fec8182f31c1131831c520572aa9abee00216)). > * **#4 `mapToCommonUpstream` throwable in unguarded streams: fixed.** `mapToCommonUpstream` and `mapToDiscoveryUpstreamData` now wrap `parseHostPort` in try/catch, return `null` + log warn on `IllegalArgumentException`, and all three stream call sites (`updateSelectorHandler`, `fetchUpstreamData`, `onDiscoveryUpstreamUpdated`) `.filter(Objects::nonNull)`. `mapToDiscoveryUpstreamData`'s null-url path now returns `null` instead of throwing ([d05fec8](https://github.com/apache/shenyu/commit/d05fec8182f31c1131831c520572aa9abee00216)). > * **#5 non-numeric port `NumberFormatException`: partially fixed.** Blank/null is now a typed `IllegalArgumentException`. A non-numeric port (`example.com:abc`) still makes `normalizeUrl`'s `Integer.parseInt(parts[1])` throw `NumberFormatException`. In the transfer/stream paths that's now harmless (NFE is a subclass of IAE, caught and skipped), but the direct `normalizeUrl` call in `nativeCreateOrUpdate` still surfaces an untyped NFE → 500. Minor — consider validating the port token in `parseHostPort` (throw typed IAE) so the service path fails cleanly too. Not a blocker. > * **#6 IPv6 coverage stops at admin/common (`ShenyuResolverHelper`/`PrxInfoUtil` still `split(":")`): not addressed.** Out of scope for this PR as flagged; please open a follow-up for the grpc/tars gateway-side consumers, or note in the PR description that this PR scopes to admin/common. > > One thing to look at before merge: the `build` matrix is currently red on all OS jobs (windows + ubuntu 17/19/20/21), but it's **not a code failure** — the log shows `##[error]The operation was canceled.` ~7 min in, with every surefire suite passing (`Tests run: X, Failures: 0, Errors: 0`) and no compile errors. It looks like a job timeout/cancellation, not a regression from these commits. Please re-run CI; if it reproduces, the `build` workflow's `timeout-minutes` may need bumping (the reactor is large and shared runners are slow). Once `build` is green I'm happy to approve — no code blockers remain. Hi,I have fixed the issues you mentioned. Please take a look when you have time. -- 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]
