Aias00 commented on code in PR #7269:
URL: https://github.com/apache/shenyu/pull/7269#discussion_r4110184944
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DiscoveryUpstreamServiceImpl.java:
##########
@@ -332,21 +334,26 @@ public ConfigImportResult importData(final String
namespace, final List<Discover
}
private void fetchAll(final String discoveryHandlerId) {
- List<DiscoveryUpstreamDO> discoveryUpstreamDOS =
discoveryUpstreamMapper.selectByDiscoveryHandlerId(discoveryHandlerId);
+ final List<DiscoveryUpstreamDO> discoveryUpstreamDOS =
discoveryUpstreamMapper.selectByDiscoveryHandlerId(discoveryHandlerId);
DiscoveryHandlerDO discoveryHandlerDO =
discoveryHandlerMapper.selectById(discoveryHandlerId);
+ Assert.notNull(discoveryHandlerDO, "Discovery handler does not exist:
" + discoveryHandlerId);
Review Comment:
Non-blocking, but worth deciding before this ships: all four callers reach
`fetchAll` **after** they have already touched the database - `updateBatch`
(DiscoveryUpstreamServiceImpl.java:112-124) deletes and re-inserts the upstream
rows, `create` (:209) and `update` (:223) insert/update first. So when one of
these asserts fires, admin keeps the row it wrote and the discovery processor
never sees it, i.e. database and gateway disagree.
This is the same outcome the NPE produced before, so it is not a regression,
but that was the old failure being invisible. Now that it is a typed
`ValidFailException` that reaches the REST layer cleanly (which is the
improvement I want), the "row persisted, push skipped" state becomes visible
too - either validate before the write, or say explicitly in the method javadoc
that this method assumes the row is committed and only guarantees the push did
not happen.
--
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]
