You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "lujiajing1126 (via GitHub)" <gi...@apache.org> on 2023/04/16 07:36:31 UTC
[GitHub] [skywalking-banyandb] lujiajing1126 opened a new pull request, #265: reload topn
lujiajing1126 opened a new pull request, #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265
In PR#239, I've moved `startSteamingManager` to the `service.Serve()` method which causes issue for TopN/measure reloading.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes https://github.com/apache/skywalking/issues/<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169436169
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage. It will cause deadlock in some corner cases!
A possible solution is to do a sanity check once after all listeners are setup.
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage! It will cause deadlock in some corner cases!
A possible solution is to do a sanity check once after all listeners are setup.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169436169
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage! It will cause deadlock in some corner cases!
A possible solution is to do a sanity check once in an async way after all listeners are setup. And we add `Eventually` blocks to these test cases to wait for all measures to be created.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1168815995
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> Could you tell me why you're copying codes here? You have already created measures for processing topNAggregation creation events.
If you remember, `preloadMeasure` and `preloadMeasure` services (specifically the `preRun` stage) are started just before the `measure`/`stream` service. So no listener/callback is setup when these schemas are installed. This could ensure that every loaded schema will take effect as soon as the `measure` and `stream` starts. I guess it is your motivation to do so at the beginning.
> If the issue involves resolving flaky scenarios, using the 'eventually' function may be helpful.
Not related to this scenario at all. But of course, we could move `preload*` after `measure` and `stream` modules. But we have to wait every actual storage modules are ready.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #265: Fix TopN reload issue
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169407057
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
There is a bug in the system that needs to be addressed. If we create a topNAggregation without creating the corresponding measure, either due to a crash or normal shutdown, the "measure" will not be built after rebooting.
To fix this issue, we need to create these measures during the `PreRun` phase of the measure service. This means that there are two ways to create topNAggregation's measures:
1. By handling topNAggregation create or update events.
2. By booting up the measure service.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #265: Fix TopN reload issue
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169862380
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> > To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
>
> As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage! It will cause deadlock in some corner cases!
No, you mixed up these two stages. During the PreRun stage, the creation event of a measure will be ignored because the handler is not registered. So, where did the deadlock come from?
The critical issue is not the test case itself, but rather the fact that in a real scenario, your current implementation will result in missing corresponding measures. This outcome is completely unacceptable.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1168815995
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> Could you tell me why you're copying codes here? You have already created measures for processing topNAggregation creation events.
If you remember, `preloadMeasure` and `preloadMeasure` services (specifically the `preRun` stage) are started just before the `measure`/`stream` service. So no listener/callback is setup when these schemas are installed. This could ensure that every loaded schema will take effect as soon as the `measure` and `stream` starts. I guess it is your motivation to do so at the beginning.
> If the issue involves resolving flaky scenarios, using the 'eventually' function may be helpful.
No related to this scenario at all. But of course, we could move `preload*` after `measure` and `stream` modules. But we have to wait every actual storage modules are ready.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] codecov-commenter commented on pull request #265: Fix TopN reload issue
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#issuecomment-1510359238
## [Codecov](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#265](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e4e9829) into [main](https://codecov.io/gh/apache/skywalking-banyandb/commit/09925ff9207abed808f4333f17b88a282cfa77c9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (09925ff) will **decrease** coverage by `0.50%`.
> The diff coverage is `10.00%`.
```diff
@@ Coverage Diff @@
## main #265 +/- ##
==========================================
- Coverage 46.87% 46.37% -0.50%
==========================================
Files 89 89
Lines 9112 9102 -10
==========================================
- Hits 4271 4221 -50
- Misses 4441 4487 +46
+ Partials 400 394 -6
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [banyand/measure/measure\_topn.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZWFzdXJlL21lYXN1cmVfdG9wbi5nbw==) | `29.24% <ø> (-4.82%)` | :arrow_down: |
| [banyand/measure/metadata.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZWFzdXJlL21ldGFkYXRhLmdv) | `44.68% <6.34%> (-14.75%)` | :arrow_down: |
| [banyand/measure/measure.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZWFzdXJlL21lYXN1cmUuZ28=) | `83.33% <33.33%> (-11.91%)` | :arrow_down: |
| [banyand/measure/service.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/265?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9tZWFzdXJlL3NlcnZpY2UuZ28=) | `74.02% <100.00%> (+5.70%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://codecov.io/gh/apache/skywalking-banyandb/pull/265/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169436169
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage! It will cause deadlock in some corner cases!
A possible solution is to do a sanity check once in an async way after all listeners are setup. And we add `Eventually` blocks to wait for all measures to be created.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1168815995
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> Could you tell me why you're copying codes here? You have already created measures for processing topNAggregation creation events.
If you remember, `preloadMeasure` and `preloadMeasure` services (specifically the `preload` stage) are started just before the `measure`/`stream` service. So no listener/callback is setup when these schemas are installed. This could ensure that every loaded schema will take effect as soon as the `measure` and `stream` starts. I guess it is your motivation to do so at the beginning.
> If the issue involves resolving flaky scenarios, using the 'eventually' function may be helpful.
No related to this scenario at all. But of course, we could move `preload*` after `measure` and `stream` modules. But we have to wait every actual storage modules are ready.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169436169
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage!
A possible solution is to do a sanity check once after all listeners are setup.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1169436169
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
> To fix this issue, we need to create these measures during the `PreRun` phase of the measure service.
As we've discussed multiple times, it is not feasible. Since watcher is created in the `Serve()` stage, we cannot do this in this `preRun()` stage! It will cause deadlock in some corner cases!
A possible solution is to do a sanity check once in an async way after all listeners are setup.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily merged pull request #265: Fix TopN reload issue
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #265: Fix TopN reload issue
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1168630727
##########
banyand/measure/metadata.go:
##########
@@ -115,6 +121,13 @@ func (sr *schemaRepo) OnAddOrUpdate(metadata schema.Metadata) {
})
}
case schema.KindTopNAggregation:
+ // createOrUpdate TopN schemas in advance
+ err := sr.createOrUpdateTopNMeasure(metadata.Spec.(*databasev1.TopNAggregation))
Review Comment:
When the "TopnAggregation" is deleted, the corresponding measure should also be removed.
##########
pkg/test/measure/etcd.go:
##########
@@ -66,13 +71,80 @@ func PreloadSchema(e schema.Registry) error {
return errors.WithStack(err)
}
if err := loadSchema(topNAggregationDir, &databasev1.TopNAggregation{}, func(topN *databasev1.TopNAggregation) error {
- return e.CreateTopNAggregation(context.TODO(), topN)
+ return multierr.Append(e.CreateTopNAggregation(context.TODO(), topN), createOrUpdateTopNMeasure(e, topN))
}); err != nil {
return errors.WithStack(err)
}
return nil
}
+func createOrUpdateTopNMeasure(reg schema.Registry, topNSchema *databasev1.TopNAggregation) error {
Review Comment:
Could you tell me why you're copying codes here? You have already created measures for processing topNAggregation creation events.
If the issue involves resolving flaky scenarios, using the 'eventually' function may be helpful.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] lujiajing1126 commented on a diff in pull request #265: Fix TopN reload issue
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #265:
URL: https://github.com/apache/skywalking-banyandb/pull/265#discussion_r1168800603
##########
banyand/measure/metadata.go:
##########
@@ -115,6 +121,13 @@ func (sr *schemaRepo) OnAddOrUpdate(metadata schema.Metadata) {
})
}
case schema.KindTopNAggregation:
+ // createOrUpdate TopN schemas in advance
+ err := sr.createOrUpdateTopNMeasure(metadata.Spec.(*databasev1.TopNAggregation))
Review Comment:
Fixed
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org