Aias00 commented on code in PR #7282:
URL: https://github.com/apache/shenyu/pull/7282#discussion_r4110028197
##########
shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-sofa/src/main/java/org/apache/shenyu/plugin/sofa/handler/SofaPluginDataHandler.java:
##########
@@ -27,13 +27,24 @@
import org.apache.shenyu.common.utils.Singleton;
import org.apache.shenyu.plugin.sofa.cache.ApplicationConfigCache;
+import com.google.common.collect.Maps;
+
+import java.util.Map;
import java.util.Objects;
/**
* The type sofa plugin data handler.
*/
public class SofaPluginDataHandler implements PluginDataHandler {
+ /**
+ * Last seen sofa upstream config per selector id. The reference cache's
+ * upstream map is keyed by the full reference cache key (selector id,
+ * metadata path, protocol and registry hash), so the last-seen config is
+ * tracked here to detect whether a selector update really changed it.
+ */
+ private static final Map<String, SofaUpstream> SELECTOR_UPSTREAM_MAP =
Maps.newConcurrentMap();
Review Comment:
Non-blocking: Guava is currently used in this module only from tests, so
`Maps.newConcurrentMap()` pulls a direct Guava dependency into main code just
to build a map. `new ConcurrentHashMap<>()` would be equivalent here and keeps
the dependency surface unchanged (it also avoids a
`maven-dependency-plugin:analyze` complaint if the module does not declare
Guava explicitly).
##########
shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-sofa/src/main/java/org/apache/shenyu/plugin/sofa/handler/SofaPluginDataHandler.java:
##########
@@ -54,19 +65,26 @@ public void handlerPlugin(final PluginData pluginData) {
@Override
public void handlerSelector(final SelectorData selectorData) {
SofaUpstream nCacheUpstreams =
GsonUtils.getInstance().fromJson(selectorData.getHandle(), SofaUpstream.class);
- SofaUpstream oCacheUpstream =
ApplicationConfigCache.getInstance().getUpstream(selectorData.getId());
+ SofaUpstream oCacheUpstream =
SELECTOR_UPSTREAM_MAP.get(selectorData.getId());
if (!Objects.equals(nCacheUpstreams, oCacheUpstream)) {
Review Comment:
Non-blocking: this comparison is the whole mechanism now, so it is worth
recording what it rests on. `SofaUpstream.equals` covers `register`, `appName`,
`protocol`, `upstreamUrl`, `port` and `gray` but ignores `weight`, `warmup`,
`status` and `timestamp` (SofaUpstream.java:171-185), while
`generateUpstreamCacheKey` depends only on selectorId + path + `protocol` +
`md5Hex(register)` (ApplicationConfigCache.java:239-251). So the covered fields
are sufficient today and the ignored ones are correctly ignored - that
alignment is what makes the no-op-on-unchanged case safe. Please add a line to
the javadoc above `SELECTOR_UPSTREAM_MAP` saying the equality must keep
covering `protocol` and `register`; otherwise a future trim of `equals` would
silently bring back the always-invalidate behaviour with green tests.
--
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]
