You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/05/29 08:03:30 UTC
[GitHub] [skywalking] andyzzl opened a new issue #7037: A question about MetricsPersistentWorker context map
andyzzl opened a new issue #7037:
URL: https://github.com/apache/skywalking/issues/7037
Please answer these questions before submitting your issue.
- Why do you submit this issue?
- [x] Question or discussion
- [ ] Bug
- [ ] Requirement
- [ ] Feature or performance improvement
___
### Question
- What do you want to know?
Hi, I have a question about the code of `MetricsPersistentWorker`.
```java
private void flushDataToStorage(List<Metrics> metricsList,
List<PrepareRequest> prepareRequests) {
try {
loadFromStorage(metricsList);
for (Metrics metrics : metricsList) {
Metrics cachedMetrics = context.get(metrics);
if (cachedMetrics != null) {
/*
* If the metrics is not supportUpdate, defined through MetricsExtension#supportUpdate,
* then no merge and further process happens.
*/
if (!supportUpdate) {
continue;
}
/*
* Merge metrics into cachedMetrics, change only happens inside cachedMetrics.
*/
final boolean isAbandoned = !cachedMetrics.combine(metrics);
if (isAbandoned) {
continue;
}
cachedMetrics.calculate();
prepareRequests.add(metricsDAO.prepareBatchUpdate(model, cachedMetrics));
nextWorker(cachedMetrics);
} else {
metrics.calculate();
prepareRequests.add(metricsDAO.prepareBatchInsert(model, metrics));
nextWorker(metrics);
}
/*
* The `metrics` should be not changed in all above process. Exporter is an async process.
*/
nextExportWorker.ifPresent(exportEvenWorker -> exportEvenWorker.in(
new ExportEvent(metrics, ExportEvent.EventType.INCREMENT)));
}
} catch (Throwable t) {
log.error(t.getMessage(), t);
} finally {
metricsList.clear();
}
}
```
In the function `flushDataToStorage`, a metrics object which not stored in `context` is considered as a new metrics, otherwise, it needs to be combined with the old metrics in `context`. After `prepareRequests` of a new metrics generated, we don't put this new metrics into `context`. So in the next run of `PersistenceTimer`, when a metrics with same id comes, we have to query db again. Is there some problems cause that we can't put a new metrics object into context?
Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] andyzzl commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
andyzzl commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850799767
After prepareRequests generated, we can put it into context, both db and context are updated. So we don’t need to query db in next run.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] andyzzl commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
andyzzl commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850794949
Sorry I didn't make it clear. I mean after `loadFromStorage` called, if a metrics can't be found in `context` still, it is a new metrics.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng closed issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #7037:
URL: https://github.com/apache/skywalking/issues/7037
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850797701
> Sorry I didn't make it clear. I mean after `loadFromStorage` called, if a metrics can't be found in `context` still, it is a new metrics.
Why it isn't? It means you need to insert it into the database, rather than updating.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng removed a comment on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng removed a comment on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850797569
> Sorry I didn't make it clear. I mean after `loadFromStorage` called, if a metrics can't be found in `context` still, it is a new metrics.
Currently, at least loading once is a way to measure this is a hot data, not a waste of memory cost. Such as an endpoint having a parameter in it, it wouldn't have a chance into the cache. This is my understanding.
Of course, this is not my code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850792920
> a metrics object which not stored in context is considered as a new metrics, otherwise, it needs to be combined with the old metrics in context
This is not a correct conclusion. Read `#loadFromStorage`, `context#put` is called if metrics could be loaded from database.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] andyzzl commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
andyzzl commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850805217
Oh, I never thought of this situation. It do cost too much memory. Thank you for your explanation.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850797569
> Sorry I didn't make it clear. I mean after `loadFromStorage` called, if a metrics can't be found in `context` still, it is a new metrics.
Currently, at least loading once is a way to measure this is a hot data, not a waste of memory cost. Such as an endpoint having a parameter in it, it wouldn't have a chance into the cache. This is my understanding.
Of course, this is not my code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #7037: A question about MetricsPersistentWorker context map
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7037:
URL: https://github.com/apache/skywalking/issues/7037#issuecomment-850801222
That depends on the context. I deleted a comment about this.
Thinking about this, if your service includes parameterized URI, such as `/prod/{id}/create`, but you don't always have grouping rules, or SpringMVC(even you have, there is Gateway before your Spring controller codes), then every request could create different endpoints. All these endpoints have one access but cost memory for nearly 1 minute.(context only has an expired period).
Considering this, do you still want to add this logic?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org