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 2022/07/29 10:04:02 UTC
[GitHub] [skywalking-infra-e2e] chunriyeqiongsaigao opened a new pull request, #82: support verify cases concurrently.
chunriyeqiongsaigao opened a new pull request, #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82
first commit.
--
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-infra-e2e] fgksgf commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r945300553
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,87 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// concurrentErrors store errors that occurred when verifying cases in goroutines.
+type concurrentErrors struct {
+ errs *multierror.Error
+ mutex sync.Mutex
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
+
+func concurrentSafeErrAppend(concurrentError *concurrentErrors, err error) {
+ concurrentError.mutex.Lock()
+ concurrentError.errs = multierror.Append(concurrentError.errs, err)
+ concurrentError.mutex.Unlock()
+}
+
+func check(stopChan chan bool, goroutineNum int) bool {
+ count := 0
+ for {
+ v, ok := <-stopChan
+ if ok {
+ if !v {
+ return false
+ }
+
+ count++
+ }
+
+ if count == goroutineNum {
+ return true
+ }
+ }
+}
+
+func concurrentVerifySingleCase(idx int, v config.VerifyCase, errs *concurrentErrors, verify verifyInfo, wg *sync.WaitGroup, stopChan chan bool) {
+ var err error
+
+ defer func() {
+ if verify.failFast {
+ if err != nil {
+ concurrentSafeErrAppend(errs, err)
+ stopChan <- false
+ } else {
+ stopChan <- true
Review Comment:
It's confusing here, because when we send `false` to `stopChan`, the program should exit.
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,87 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// concurrentErrors store errors that occurred when verifying cases in goroutines.
+type concurrentErrors struct {
+ errs *multierror.Error
+ mutex sync.Mutex
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
+
+func concurrentSafeErrAppend(concurrentError *concurrentErrors, err error) {
+ concurrentError.mutex.Lock()
+ concurrentError.errs = multierror.Append(concurrentError.errs, err)
+ concurrentError.mutex.Unlock()
+}
+
+func check(stopChan chan bool, goroutineNum int) bool {
+ count := 0
+ for {
+ v, ok := <-stopChan
+ if ok {
+ if !v {
+ return false
+ }
+
+ count++
+ }
Review Comment:
We can use `for-range` here and make variables more informative, like:
```go
for shouldExit := range stopChan {
if shouldExit {
// ...
}
}
```
--
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-infra-e2e] chunriyeqiongsaigao commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
chunriyeqiongsaigao commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200071766
> Concurrency verification, do you mean cases would be run in the same infra? Such as same k8s/kind. This seems not very reasonable from test theory. Or do you create 2 kind clusters or docker compose to do so? This seema tricky as we may face port conflict or local file lock due to race conditions.
Thanks for your advice! I will try my best to improve the Concurrency verification.
--
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-infra-e2e] fgksgf commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200085874
> Concurrency verification, do you mean cases would be run in the same infra? Such as same k8s/kind. This seems not very reasonable from test theory. Or do you create 2 kind clusters or docker compose to do so? This seema tricky as we may face port conflict or local file lock due to race conditions.
This is a sub-task of Summer 2022, and I originally wanted to add a feature that cases in verify stage (or verify steps) can run concurrently (in the same infra).
Since in some scenarios, all steps are just querying data and their order don't matter, like [this example](https://github.com/apache/skywalking/blob/554c55457dcf9aca4d896c2af32ea8a3e6778ebd/test/e2e-v2/cases/simple/simple-cases.yaml#L16). Users can choose whether or not to enable this feature, so I think it's ok to have this feature.
--
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-infra-e2e] wu-sheng commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200079871
My question is, do you add cases running concurrently or only verify steps?
--
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-infra-e2e] wu-sheng commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200014146
Could you put the introduction about what is this PR about in the description?
--
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-infra-e2e] wu-sheng commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200087482
OK, then I don't have many concerns. Just want to remind, if this test is running in GHA or similar CI env, the CPU resources would be very limited, 3 cores for whole env. Even you have concurrency support, the improvement could even be negative, I am afraid.
This is not a block, but we should think carefully and run some verification, if the plan is we are going to eat our dog food.
--
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-infra-e2e] fgksgf commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r933835926
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,38 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
Review Comment:
1. It would be better to add a comment for `ErrContain`
2. The name `errAddr` is confusing, what does it mean?
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,38 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
+
+func concurrentVerifySingleCase(idx int, v config.VerifyCase, errContain *ErrContain, retry int, interval time.Duration, wg *sync.WaitGroup) {
+ if v.GetExpected() == "" {
+ errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
+ logger.Log.Warnf(errMsg)
+ errContain.mutex.Lock()
+ errContain.errAddr = multierror.Append(errContain.errAddr, errors.New(errMsg))
+ errContain.mutex.Unlock()
+ wg.Done()
Review Comment:
This code logic is repeated, I think we can put them to a defer function
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,38 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
+
+func concurrentVerifySingleCase(idx int, v config.VerifyCase, errContain *ErrContain, retry int, interval time.Duration, wg *sync.WaitGroup) {
+ if v.GetExpected() == "" {
+ errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
+ logger.Log.Warnf(errMsg)
+ errContain.mutex.Lock()
+ errContain.errAddr = multierror.Append(errContain.errAddr, errors.New(errMsg))
+ errContain.mutex.Unlock()
+ wg.Done()
+ return
+ }
+
+ for current := 1; current <= retry; current++ {
+ if err := verifySingleCase(v.GetExpected(), v.GetActual(), v.Query); err == nil {
+ break
+ } else if current != retry {
+ logger.Log.Warnf("verify case[%v] failure, will continue retry, %v", idx, err)
+ time.Sleep(interval)
+ } else {
+ errContain.mutex.Lock()
+ errContain.errAddr = multierror.Append(errContain.errAddr, err)
+ errContain.mutex.Unlock()
+ }
+ }
Review Comment:
Besides, there is a tricky thing you need to consider: the fail-fast (or not) in concurrent mode.
--
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-infra-e2e] wu-sheng merged pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82
--
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-infra-e2e] wu-sheng commented on pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#issuecomment-1200015375
Concurrency verification, do you mean cases would be run in the same infra? Such as same k8s/kind. This seems not very reasonable from test theory.
Or do you create 2 kind clusters or docker compose to do so? This seema tricky as we may face port conflict or local file lock due to race conditions.
--
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-infra-e2e] fgksgf commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r945133905
##########
commands/verify/verify.go:
##########
@@ -96,44 +178,73 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
- }
- logger.Log.Warnf(errMsg)
- errCollection = multierror.Append(errCollection, errors.New(errMsg))
- continue
+ var Errs *multierror.Error
+
+ if concurrency {
+ var waitGroup sync.WaitGroup
+ ConcurrentErrors := concurrentErrors{
+ errs: Errs,
+ count: len(e2eConfig.Verify.Cases),
+ }
+
+ VerifyInfo := verifyInfo{
+ retryCount,
+ interval,
+ failFast,
+ }
+ stopChan := make(chan struct{})
+ waitGroup.Add(len(e2eConfig.Verify.Cases))
+
+ for idx, v := range e2eConfig.Verify.Cases {
+ go concurrentVerifySingleCase(idx, v, &ConcurrentErrors, VerifyInfo, &waitGroup, stopChan)
}
- for current := 1; current <= retryCount; current++ {
- if err := verifySingleCase(v.GetExpected(), v.GetActual(), v.Query); err == nil {
- break
- } else if current != retryCount {
- logger.Log.Warnf("verify case failure, will continue retry, %v", err)
- time.Sleep(interval)
- } else {
+ if failFast {
+ if err := check(stopChan, &ConcurrentErrors); err != nil {
+ return err
+ }
+ }
+ waitGroup.Wait()
+ Errs = ConcurrentErrors.errs
Review Comment:
```suggestion
```
Since you have passed the pointer of `Errs` to `ConcurrentErrors`, this statement is not needed.
##########
commands/verify/verify.go:
##########
@@ -96,44 +178,73 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
- }
- logger.Log.Warnf(errMsg)
- errCollection = multierror.Append(errCollection, errors.New(errMsg))
- continue
+ var Errs *multierror.Error
Review Comment:
```suggestion
var errs *multierror.Error
```
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,87 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// concurrentErrors store errors that occurred when verifying cases in goroutines.
+type concurrentErrors struct {
+ errs *multierror.Error
+ mutex sync.Mutex
+ count int
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
+
+func concurrentSafeErrAppend(concurrentError *concurrentErrors, err error) {
+ concurrentError.mutex.Lock()
+ concurrentError.errs = multierror.Append(concurrentError.errs, err)
+ concurrentError.mutex.Unlock()
+}
+
+func concurrentSafeCountLess(concurrentError *concurrentErrors) {
+ concurrentError.mutex.Lock()
+ concurrentError.count--
+ concurrentError.mutex.Unlock()
+}
+
+func check(stopChan chan struct{}, concurrentErrors *concurrentErrors) error {
+ for {
+ if concurrentErrors.count == 0 {
+ break
+ }
+ _, ok := <-stopChan
Review Comment:
It will be blocked here if all concurrent cases success. Maybe you can refer to [this](https://stackoverflow.com/questions/45500836/close-multiple-goroutine-if-an-error-occurs-in-one-in-go).
--
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-infra-e2e] fgksgf commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r933988889
##########
commands/verify/verify.go:
##########
@@ -96,44 +154,75 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
+
+ var errCollection multierror.Error
+
+ errContain := ErrContain{
+ errAddr: &errCollection,
+ }
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
+ if concurrency {
+ var waitGroup sync.WaitGroup
+ VerifyInfo := verifyInfo{
+ retryCount,
+ interval,
+ failFast,
+ }
+ chanBool := make(chan bool, 1)
+ waitGroup.Add(len(e2eConfig.Verify.Cases))
+
+ for idx, v := range e2eConfig.Verify.Cases {
+ go concurrentVerifySingleCase(idx, v, &errContain, VerifyInfo, &waitGroup, chanBool)
+ }
+
+ if failFast {
+ for {
+ result := <-chanBool
+ if result {
+ return errContain.errAddr.ErrorOrNil()
+ }
Review Comment:
We can use a goroutine to do this, otherwise it will blocks
--
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-infra-e2e] chunriyeqiongsaigao commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
chunriyeqiongsaigao commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r945162998
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,87 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// concurrentErrors store errors that occurred when verifying cases in goroutines.
+type concurrentErrors struct {
+ errs *multierror.Error
+ mutex sync.Mutex
+ count int
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
+
+func concurrentSafeErrAppend(concurrentError *concurrentErrors, err error) {
+ concurrentError.mutex.Lock()
+ concurrentError.errs = multierror.Append(concurrentError.errs, err)
+ concurrentError.mutex.Unlock()
+}
+
+func concurrentSafeCountLess(concurrentError *concurrentErrors) {
+ concurrentError.mutex.Lock()
+ concurrentError.count--
+ concurrentError.mutex.Unlock()
+}
+
+func check(stopChan chan struct{}, concurrentErrors *concurrentErrors) error {
+ for {
+ if concurrentErrors.count == 0 {
+ break
+ }
+ _, ok := <-stopChan
Review Comment:
Thanks! I will learn it !
--
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-infra-e2e] fgksgf commented on a diff in pull request #82: support verify cases concurrently.
Posted by GitBox <gi...@apache.org>.
fgksgf commented on code in PR #82:
URL: https://github.com/apache/skywalking-infra-e2e/pull/82#discussion_r933985435
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,63 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// ErrContain has two fields. The errAddr field is a pointer points to Err. Err's type is "multierror.Error".
+// When an error occurs during concurrently verifying cases, the process will append the new error to Err.Errors.
+// The Err.Errors is []error, which contains all errors occurring during the verification. errAddr points to Err.
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
+
+func concurrentSafeErrAppend(errContain *ErrContain, err error) {
+ errContain.mutex.Lock()
+ errContain.errAddr = multierror.Append(errContain.errAddr, err)
+ errContain.mutex.Unlock()
+}
+
+func concurrentVerifySingleCase(idx int, v config.VerifyCase, errContain *ErrContain, verify verifyInfo, wg *sync.WaitGroup, chanBool chan bool) {
Review Comment:
```go
concurrentSafeErrAppend(errContain, errors.New(errMsg))
if verify.failFast {
chanBool <- true
}
wg.Done()
```
⬆️ The above login is still repeated.
What I mean is like:
```go
func concurrentVerifySingleCase(idx int, v config.VerifyCase, errs *errorsWithLock, cfg config.Verify, wg *sync.WaitGroup, chanBool chan bool) (err Error){
defer func() {
if err != nil {
// append the error
// send the signal
}
wg.Done()
}
// ...
}
```
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,63 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// ErrContain has two fields. The errAddr field is a pointer points to Err. Err's type is "multierror.Error".
+// When an error occurs during concurrently verifying cases, the process will append the new error to Err.Errors.
+// The Err.Errors is []error, which contains all errors occurring during the verification. errAddr points to Err.
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
+
+// verifyInfo contains necessary information about verification
+type verifyInfo struct {
+ retryCount int
+ interval time.Duration
+ failFast bool
+}
Review Comment:
I don't think this struct is necessary, because we can pass `e2eConfig.Verify` to the function, right?
##########
commands/verify/verify.go:
##########
@@ -96,44 +154,75 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
+
+ var errCollection multierror.Error
+
+ errContain := ErrContain{
+ errAddr: &errCollection,
+ }
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
+ if concurrency {
+ var waitGroup sync.WaitGroup
+ VerifyInfo := verifyInfo{
+ retryCount,
+ interval,
+ failFast,
+ }
+ chanBool := make(chan bool, 1)
+ waitGroup.Add(len(e2eConfig.Verify.Cases))
+
+ for idx, v := range e2eConfig.Verify.Cases {
+ go concurrentVerifySingleCase(idx, v, &errContain, VerifyInfo, &waitGroup, chanBool)
+ }
+
+ if failFast {
+ for {
+ result := <-chanBool
+ if result {
+ return errContain.errAddr.ErrorOrNil()
+ }
}
- logger.Log.Warnf(errMsg)
- errCollection = multierror.Append(errCollection, errors.New(errMsg))
- continue
}
- for current := 1; current <= retryCount; current++ {
- if err := verifySingleCase(v.GetExpected(), v.GetActual(), v.Query); err == nil {
- break
- } else if current != retryCount {
- logger.Log.Warnf("verify case failure, will continue retry, %v", err)
- time.Sleep(interval)
- } else {
+ waitGroup.Wait()
+ } else {
+ for idx, v := range e2eConfig.Verify.Cases {
+ if v.GetExpected() == "" {
+ errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
if failFast {
- return err
+ return errors.New(errMsg)
+ }
+ logger.Log.Warnf(errMsg)
+ errContain.errAddr = multierror.Append(errContain.errAddr, errors.New(errMsg))
+ continue
+ }
+
+ for current := 1; current <= retryCount; current++ {
+ if err := verifySingleCase(v.GetExpected(), v.GetActual(), v.Query); err == nil {
+ break
+ } else if current != retryCount {
+ logger.Log.Warnf("verify case failure, will continue retry, %v", err)
+ time.Sleep(interval)
+ } else {
+ if failFast {
+ return err
+ }
+ errContain.errAddr = multierror.Append(errContain.errAddr, err)
}
- errCollection = multierror.Append(errCollection, err)
}
}
}
- return errCollection.ErrorOrNil()
+ return errContain.errAddr.ErrorOrNil()
Review Comment:
Since we passed the pointer of `errCollection` to `errContain`, no need to modify this.
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,63 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// ErrContain has two fields. The errAddr field is a pointer points to Err. Err's type is "multierror.Error".
+// When an error occurs during concurrently verifying cases, the process will append the new error to Err.Errors.
+// The Err.Errors is []error, which contains all errors occurring during the verification. errAddr points to Err.
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
Review Comment:
```suggestion
// concurrentErrors store errors that occurred when verifying cases in goroutines.
type concurrentErrors struct {
errs *multierror.Error
mutex sync.Mutex
}
```
##########
commands/verify/verify.go:
##########
@@ -96,44 +154,75 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
+
+ var errCollection multierror.Error
+
+ errContain := ErrContain{
+ errAddr: &errCollection,
+ }
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
+ if concurrency {
+ var waitGroup sync.WaitGroup
+ VerifyInfo := verifyInfo{
+ retryCount,
+ interval,
+ failFast,
+ }
+ chanBool := make(chan bool, 1)
Review Comment:
The name `chanBool` is not informative, `stopChan` may be better. Besides, we can use `chan struct{}`, you can search related articles to learn about it.
##########
commands/verify/verify.go:
##########
@@ -87,6 +88,63 @@ func verifySingleCase(expectedFile, actualFile, query string) error {
return nil
}
+// ErrContain has two fields. The errAddr field is a pointer points to Err. Err's type is "multierror.Error".
+// When an error occurs during concurrently verifying cases, the process will append the new error to Err.Errors.
+// The Err.Errors is []error, which contains all errors occurring during the verification. errAddr points to Err.
+type ErrContain struct {
+ errAddr *multierror.Error
+ mutex sync.Mutex
+}
Review Comment:
Comments should be concise and clear. No nonsense.
##########
commands/verify/verify.go:
##########
@@ -96,44 +154,75 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
+
+ var errCollection multierror.Error
+
+ errContain := ErrContain{
+ errAddr: &errCollection,
+ }
Review Comment:
This block can be moved to `if concurrency {`
##########
commands/verify/verify.go:
##########
@@ -96,44 +154,75 @@ func DoVerifyAccordingConfig() error {
e2eConfig := config.GlobalConfig.E2EConfig
retryCount := e2eConfig.Verify.RetryStrategy.Count
- if retryCount <= 0 {
- retryCount = 1
- }
+ retryCount = checkForRetryCount(retryCount)
+
interval, err := parseInterval(e2eConfig.Verify.RetryStrategy.Interval)
if err != nil {
return err
}
failFast := e2eConfig.Verify.FailFast
+ concurrency := e2eConfig.Verify.Concurrency
+
+ var errCollection multierror.Error
+
+ errContain := ErrContain{
+ errAddr: &errCollection,
+ }
- var errCollection *multierror.Error
- for idx, v := range e2eConfig.Verify.Cases {
- if v.GetExpected() == "" {
- errMsg := fmt.Sprintf("the expected data file for case[%v] is not specified\n", idx)
- if failFast {
- return errors.New(errMsg)
+ if concurrency {
+ var waitGroup sync.WaitGroup
+ VerifyInfo := verifyInfo{
+ retryCount,
+ interval,
+ failFast,
+ }
+ chanBool := make(chan bool, 1)
+ waitGroup.Add(len(e2eConfig.Verify.Cases))
+
+ for idx, v := range e2eConfig.Verify.Cases {
+ go concurrentVerifySingleCase(idx, v, &errContain, VerifyInfo, &waitGroup, chanBool)
+ }
+
+ if failFast {
+ for {
+ result := <-chanBool
+ if result {
+ return errContain.errAddr.ErrorOrNil()
+ }
Review Comment:
We can use a goroutine to do this, otherwise it will blocks
--
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