Copilot commented on code in PR #1029:
URL: https://github.com/apache/dubbo-go-samples/pull/1029#discussion_r2772305583
##########
config_center/nacos/go-server/cmd/main.go:
##########
@@ -123,3 +123,42 @@ func main() {
logger.Error(err)
}
}
+
+func publishAndWaitConfig(
+ configClient config_client.IConfigClient,
+ dataID string,
+ group string,
+ content string,
+ timeout time.Duration,
+ pollInterval time.Duration,
+) error {
+ success, err := configClient.PublishConfig(vo.ConfigParam{
+ DataId: dataID,
+ Group: group,
+ Content: content,
+ })
+ if err != nil {
+ return err
+ }
+ if !success {
+ return fmt.Errorf("publish config failed")
+ }
+
+ deadline := time.Now().Add(timeout)
+ for {
+ current, err := configClient.GetConfig(vo.ConfigParam{
+ DataId: dataID,
+ Group: group,
+ })
+ if err == nil && strings.TrimSpace(current) ==
strings.TrimSpace(content) {
+ return nil
+ }
+ if time.Now().After(deadline) {
+ if err != nil {
+ return err
+ }
+ return fmt.Errorf("wait for config center timeout")
+ }
+ time.Sleep(pollInterval)
+ }
+}
Review Comment:
The publishAndWaitConfig function is duplicated between
config_center/nacos/go-server/cmd/main.go and
config_center/nacos/go-client/cmd/main.go with identical implementation.
Consider extracting this function to a shared utility package to reduce code
duplication and make maintenance easier.
##########
config_center/zookeeper/go-server/cmd/main.go:
##########
@@ -151,9 +148,31 @@ func writeRuleToConfigCenter() error {
logger.Info("Created new configuration in config center")
}
+ if err := waitForConfigReady(c, path, valueBytes, 10*time.Second); err
!= nil {
+ return perrors.Wrap(err, "wait for config ready")
+ }
+
return nil
}
+func waitForConfigReady(c *zk.Conn, path string, expected []byte, timeout
time.Duration) error {
+ deadline := time.Now().Add(timeout)
+ expectedStr := strings.TrimSpace(string(expected))
+ for {
+ data, _, err := c.Get(path)
+ if err == nil && strings.TrimSpace(string(data)) == expectedStr
{
+ return nil
+ }
+ if time.Now().After(deadline) {
+ if err != nil {
+ return perrors.Wrap(err, "wait for config
timeout")
+ }
+ return perrors.New("wait for config timeout")
+ }
+ time.Sleep(200 * time.Millisecond)
+ }
+}
Review Comment:
The waitForConfigReady function is duplicated across three files:
config_center/zookeeper/go-server/cmd/main.go,
config_center/zookeeper/go-client/cmd/main.go, and
integrate_test/config_center/zookeeper/tests/integration/main_test.go. Consider
extracting this function to a shared utility package to reduce code duplication
and make maintenance easier.
##########
integrate_test/filter/sentinel/tests/integration/sentinel_test.go:
##########
@@ -123,42 +123,40 @@ func TestSentinelConsumerFilter_ErrorCount(t *testing.T) {
listener.OnTransformToClosedChan = make(chan struct{}, 1)
listener.OnTransformToHalfOpenChan = make(chan struct{}, 1)
circuitbreaker.RegisterStateChangeListeners(listener)
- _, err := circuitbreaker.LoadRules([]*circuitbreaker.Rule{
+ rule := &circuitbreaker.Rule{
// Statistic time span=0.9s, recoveryTimeout=0.9s,
maxErrorCount=50
- {
- Resource:
"dubbo:consumer:greet.SentinelGreetService:::GreetWithChanceOfError()",
- Strategy: circuitbreaker.ErrorCount,
- RetryTimeoutMs: 900,
- MinRequestAmount: 10,
- StatIntervalMs: 900,
- StatSlidingWindowBucketCount: 10,
- Threshold: 50,
- },
- })
+ Resource:
"dubbo:consumer:greet.SentinelGreetService:::GreetWithChanceOfError()",
+ Strategy: circuitbreaker.ErrorCount,
+ RetryTimeoutMs: 900,
+ MinRequestAmount: 10,
+ StatIntervalMs: 900,
+ StatSlidingWindowBucketCount: 10,
+ Threshold: 50,
+ }
+ _, err := circuitbreaker.LoadRules([]*circuitbreaker.Rule{rule})
assert.NoError(t, err)
pass, block :=
CallGreetFunConcurrently(greetService.GreetWithChanceOfError, "error", 1, 50)
assert.True(t, pass == 0)
assert.True(t, block == 50)
- select {
- case <-time.After(time.Second):
- t.Error()
- case <-listener.OnTransformToOpenChan:
- }
- // wait half open
- time.Sleep(time.Second)
+ waitForState(t, listener.OnTransformToOpenChan, time.Second, "open")
+ timer :=
time.NewTimer(time.Duration(rule.RetryTimeoutMs)*time.Millisecond +
200*time.Millisecond)
+ <-timer.C
+ timer.Stop()
Review Comment:
The timer created here is stopped after reading from timer.C, but this is
unnecessary and slightly inefficient. Once a timer expires and sends to its
channel, calling Stop returns false and does nothing. The Stop call should be
removed or only called in cases where the timer needs to be cancelled before
expiration.
```suggestion
```
##########
filter/sentinel/go-client/cmd/main.go:
##########
@@ -106,10 +137,20 @@ func main() {
logger.Info("call svc.GreetWithChanceOfError triggers circuit breaker
open")
CallGreetFunConcurrently(svc.GreetWithChanceOfError, "error", 1, 300)
- logger.Info("wait circuit breaker HalfOpen")
- time.Sleep(3 * time.Second)
+ if !waitForState(listener.openCh, 5*time.Second) {
+ logger.Warn("wait circuit breaker Open timeout")
+ }
+ logger.Info("wait circuit breaker HalfOpen window")
+ timer := time.NewTimer(retryTimeout + 200*time.Millisecond)
+ <-timer.C
+ timer.Stop()
Review Comment:
The timer created here is stopped after reading from timer.C, but this is
unnecessary. Once a timer expires and sends to its channel, calling Stop
returns false and does nothing. The Stop call should be removed or only called
in cases where the timer needs to be cancelled before expiration.
```suggestion
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
