Aias00 commented on code in PR #7268:
URL: https://github.com/apache/shenyu/pull/7268#discussion_r4110185013
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DiscoveryUpstreamServiceImpl.java:
##########
@@ -147,27 +149,53 @@ public String delete(final List<String> ids) {
@Override
public List<DiscoverySyncData> listAll() {
- List<DiscoveryHandlerDO> discoveryHandlerDOS =
discoveryHandlerMapper.selectAll();
- return discoveryHandlerDOS.stream().map(d -> {
- DiscoveryRelDO discoveryRelDO =
discoveryRelMapper.selectByDiscoveryHandlerId(d.getId());
- DiscoverySyncData discoverySyncData = new DiscoverySyncData();
- discoverySyncData.setPluginName(discoveryRelDO.getPluginName());
- if (StringUtils.hasLength(discoveryRelDO.getSelectorId())) {
- String selectorId = discoveryRelDO.getSelectorId();
- discoverySyncData.setSelectorId(selectorId);
- SelectorDO selectorDO = selectorMapper.selectById(selectorId);
-
discoverySyncData.setSelectorName(selectorDO.getSelectorName());
- } else {
- String proxySelectorId = discoveryRelDO.getProxySelectorId();
- discoverySyncData.setSelectorId(proxySelectorId);
- ProxySelectorDO proxySelectorDO =
proxySelectorMapper.selectById(proxySelectorId);
- discoverySyncData.setSelectorName(proxySelectorDO.getName());
+ return buildSyncData(discoveryHandlerMapper.selectAll());
+ }
+
+ private List<DiscoverySyncData> buildSyncData(final
List<DiscoveryHandlerDO> handlers) {
+ List<DiscoverySyncData> result = new ArrayList<>();
+ for (List<DiscoveryHandlerDO> batch : Lists.partition(handlers, 500)) {
+ List<String> handlerIds =
batch.stream().map(DiscoveryHandlerDO::getId).collect(Collectors.toList());
+ List<DiscoveryRelDO> relations =
discoveryRelMapper.selectByDiscoveryHandlerIds(handlerIds);
+ Set<String> selectorIds =
relations.stream().map(DiscoveryRelDO::getSelectorId).filter(StringUtils::hasLength).collect(Collectors.toSet());
+ List<String> proxyIds = relations.stream().filter(rel ->
!StringUtils.hasLength(rel.getSelectorId()))
+
.map(DiscoveryRelDO::getProxySelectorId).filter(StringUtils::hasLength).distinct().collect(Collectors.toList());
+ Map<String, SelectorDO> selectors = selectorIds.isEmpty() ?
Collections.emptyMap()
+ :
selectorMapper.selectByIdSet(selectorIds).stream().collect(Collectors.toMap(SelectorDO::getId,
Function.identity()));
+ Map<String, ProxySelectorDO> proxies = proxyIds.isEmpty() ?
Collections.emptyMap()
+ :
proxySelectorMapper.selectByIds(proxyIds).stream().collect(Collectors.toMap(ProxySelectorDO::getId,
Function.identity()));
+ Map<String, List<DiscoveryUpstreamData>> upstreams =
discoveryUpstreamMapper.selectByDiscoveryHandlerIds(handlerIds).stream()
+
.collect(Collectors.groupingBy(DiscoveryUpstreamDO::getDiscoveryHandlerId,
+
Collectors.mapping(DiscoveryTransfer.INSTANCE::mapToData,
Collectors.toList())));
+ Map<String, DiscoveryRelDO> relationByHandler = relations.stream()
+
.collect(Collectors.toMap(DiscoveryRelDO::getDiscoveryHandlerId,
Function.identity()));
Review Comment:
Suggestion: `Collectors.toMap` with no merge function throws
`IllegalStateException: Duplicate key` when two relation rows exist for one
handler, and nothing prevents that - the `discovery_rel` primary key is `id`,
there is **no** unique constraint on `discovery_handler_id`
(db/init/mysql/schema.sql:2463-2473 and the H2 schema agree). When it happens
it takes down `listAll()` for the whole discovery sync, which is every
bootstrap refresh, not just that handler.
The old code failed too (a single-row select would raise
`TooManyResultsException`), so this is not a regression - but since you are
rewriting this anyway, one lambda makes it survivable:
```java
.collect(Collectors.toMap(DiscoveryRelDO::getDiscoveryHandlerId,
Function.identity(), (existing, ignored) -> existing));
```
Picking `existing` keeps "first wins", which matches the old behaviour for
handlers that do have a single row.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DiscoveryUpstreamServiceImpl.java:
##########
@@ -147,27 +149,53 @@ public String delete(final List<String> ids) {
@Override
public List<DiscoverySyncData> listAll() {
- List<DiscoveryHandlerDO> discoveryHandlerDOS =
discoveryHandlerMapper.selectAll();
- return discoveryHandlerDOS.stream().map(d -> {
- DiscoveryRelDO discoveryRelDO =
discoveryRelMapper.selectByDiscoveryHandlerId(d.getId());
- DiscoverySyncData discoverySyncData = new DiscoverySyncData();
- discoverySyncData.setPluginName(discoveryRelDO.getPluginName());
- if (StringUtils.hasLength(discoveryRelDO.getSelectorId())) {
- String selectorId = discoveryRelDO.getSelectorId();
- discoverySyncData.setSelectorId(selectorId);
- SelectorDO selectorDO = selectorMapper.selectById(selectorId);
-
discoverySyncData.setSelectorName(selectorDO.getSelectorName());
- } else {
- String proxySelectorId = discoveryRelDO.getProxySelectorId();
- discoverySyncData.setSelectorId(proxySelectorId);
- ProxySelectorDO proxySelectorDO =
proxySelectorMapper.selectById(proxySelectorId);
- discoverySyncData.setSelectorName(proxySelectorDO.getName());
+ return buildSyncData(discoveryHandlerMapper.selectAll());
+ }
+
+ private List<DiscoverySyncData> buildSyncData(final
List<DiscoveryHandlerDO> handlers) {
+ List<DiscoverySyncData> result = new ArrayList<>();
+ for (List<DiscoveryHandlerDO> batch : Lists.partition(handlers, 500)) {
+ List<String> handlerIds =
batch.stream().map(DiscoveryHandlerDO::getId).collect(Collectors.toList());
+ List<DiscoveryRelDO> relations =
discoveryRelMapper.selectByDiscoveryHandlerIds(handlerIds);
+ Set<String> selectorIds =
relations.stream().map(DiscoveryRelDO::getSelectorId).filter(StringUtils::hasLength).collect(Collectors.toSet());
+ List<String> proxyIds = relations.stream().filter(rel ->
!StringUtils.hasLength(rel.getSelectorId()))
+
.map(DiscoveryRelDO::getProxySelectorId).filter(StringUtils::hasLength).distinct().collect(Collectors.toList());
+ Map<String, SelectorDO> selectors = selectorIds.isEmpty() ?
Collections.emptyMap()
+ :
selectorMapper.selectByIdSet(selectorIds).stream().collect(Collectors.toMap(SelectorDO::getId,
Function.identity()));
+ Map<String, ProxySelectorDO> proxies = proxyIds.isEmpty() ?
Collections.emptyMap()
+ :
proxySelectorMapper.selectByIds(proxyIds).stream().collect(Collectors.toMap(ProxySelectorDO::getId,
Function.identity()));
+ Map<String, List<DiscoveryUpstreamData>> upstreams =
discoveryUpstreamMapper.selectByDiscoveryHandlerIds(handlerIds).stream()
+
.collect(Collectors.groupingBy(DiscoveryUpstreamDO::getDiscoveryHandlerId,
+
Collectors.mapping(DiscoveryTransfer.INSTANCE::mapToData,
Collectors.toList())));
+ Map<String, DiscoveryRelDO> relationByHandler = relations.stream()
+
.collect(Collectors.toMap(DiscoveryRelDO::getDiscoveryHandlerId,
Function.identity()));
+ for (DiscoveryHandlerDO handler : batch) {
+ DiscoveryRelDO relation =
relationByHandler.get(handler.getId());
+ if (Objects.isNull(relation)) {
Review Comment:
Suggestion (non-blocking): skipping instead of throwing is the right
trade-off - a handler with a dangling relation used to NPE and break the sync
for everyone - but it is completely silent. Operators whose upstreams vanished
from the gateway have nothing to correlate against, and depending on the sync
protocol an absent group can read as "this selector has no upstreams".
A `LOG.warn` carrying the handler id and the reason (relation missing /
selector missing / proxy selector missing) before each `continue` would make it
diagnosable. Same for `500` below: it deserves a named constant with a line on
why that number.
--
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]
