kezhenxu94 commented on a change in pull request #8193:
URL: https://github.com/apache/skywalking/pull/8193#discussion_r757836878
##########
File path:
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/AccessLogServiceGRPCHandler.java
##########
@@ -105,7 +110,7 @@ public AccessLogServiceGRPCHandler(ModuleManager manager,
public void onNext(StreamAccessLogsMessage message) {
HistogramMetrics.Timer timer = histogram.createTimer();
try {
- if (isFirst) {
+ if (isFirst || (alwaysAnalyzeIdentity &&
message.getIdentifier() != null)) {
Review comment:
> What you are concerned and asked about actually is already there after
local balancer added. So, generally, you need a tradeoff, because trying to
balance 2k+ envoy instances, the streaming must be merged one way or another.
I agree we need tradeoff and I've also given another merging strategy for
discussion, which I think might be better
https://github.com/apache/skywalking/pull/8193#issuecomment-980766033
> So, generally, your asking about more resource cost at OAP side is true,
but not because of this flag, but load balancing mechanism.
I doubt this and need to confirm with @mrproliu .
Despite of all above, `if (isFirst || (alwaysAnalyzeIdentity &&
message.getIdentifier() != null))` is buggy because `message.getIdentifier()`
is never null, it returns a default instance if `identifier == null` so you
might always get `role == NONE`
--
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]
