Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: rename the variables in TCP reconnect function and some default constants #84

Merged
merged 8 commits into from
Jul 16, 2024
Merged

Improvement: rename the variables in TCP reconnect function and some default constants #84

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
972421e
refact:rename the variable name in TCP reconnect
No-SilverBullet Jun 16, 2024
38deaf2
refact:rename the variable name in TCP reconnect
No-SilverBullet Jun 16, 2024
8529f81
improvement:add new client attribute maxReconnectAttempts.
No-SilverBullet Jun 18, 2024
5a06061
refact:update annotation
No-SilverBullet Jun 18, 2024
5bd3869
change the initialization method of some variables in the reConnect()…
No-SilverBullet Jun 26, 2024
3f88499
improve:add error handling in SetReadDeadline and SetWriteDeadline an…
No-SilverBullet Jul 4, 2024
12033ac
improve:add TcpMaxReconnectAttempts field in GettySessionParam which …
No-SilverBullet Jul 4, 2024
bace946
change TcpMaxReconnectAttempts json tag
No-SilverBullet Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 23 additions & 18 deletions transport/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ import (
)

const (
reconnectInterval = 3e8 // 300ms
connectInterval = 5e8 // 500ms
connectTimeout = 3e9
maxTimes = 10
defaultReconnectInterval = 3e8 // 300ms
connectInterval = 5e8 // 500ms
connectTimeout = 3e9
defaultMaxReconnectAttempts = 50
maxBackOffTimes = 10
Comment on lines +45 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Incomplete Renaming of Constants

The constants have been partially renamed to improve readability. However, occurrences of the old constant name reconnectInterval still exist in the codebase. Please ensure all references to the old constant names are updated to the new names.

  • transport/options.go
  • transport/client.go
  • transport/options_test.go
Analysis chain

Renamed Constants for Clarity.

The constants have been renamed to improve readability. Ensure that all references to these constants in the codebase are updated accordingly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all references to the renamed constants are updated in the codebase.

# Test: Search for the old constant names. Expect: No occurrences of old names.
rg --type go 'reconnectInterval|maxTimes'

Length of output: 783

Script:

#!/bin/bash
# Description: Verify that the new constant names are used correctly in the codebase.

# Test: Search for the new constant names in the codebase.
rg --type go 'defaultReconnectInterval|maxBackOffTimes'

Length of output: 384

)

var (
Expand Down Expand Up @@ -423,33 +424,37 @@ func (c *client) RunEventLoop(newSession NewSessionCallback) {
// a for-loop connect to make sure the connection pool is valid
func (c *client) reConnect() {
var (
num, max, times, interval int
maxDuration int64
sessionNum, reconnectAttempts int
maxReconnectInterval int64
Comment on lines +432 to +433
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Incomplete Variable Renaming

The variables num, times, and maxDuration have not been fully renamed to sessionNum, reconnectAttempts, and maxReconnectInterval respectively in the codebase. The following occurrences in transport/client.go still need to be updated:

  • num:
    • transport/client.go: if c.number <= 0 || c.addr == "" {
    • transport/client.go: panic(fmt.Sprintf("client type:%s, @connNum:%d, @serverAddr:%s", t, c.number, c.addr))
    • transport/client.go: c.ssMap = make(map[Session]struct{}, c.number)
    • transport/client.go: var num int
    • transport/client.go: num = len(c.ssMap)
    • transport/client.go: return num
    • transport/client.go: connPoolSize := c.number

Please ensure that all references to these variables are updated accordingly to maintain consistency.

Analysis chain

Renamed Variables for Clarity.

The variables num, times, and maxDuration have been renamed to sessionNum, reconnectAttempts, and maxReconnectInterval respectively. Ensure that all references to these variables in the codebase are updated accordingly.

The search results indicate that the old variable names num, times, and maxDuration still exist in the codebase. This suggests that not all references have been updated. To verify this, we need to check the specific lines in transport/client.go and other related files to confirm whether the renaming was applied correctly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all references to the renamed variables are updated in the codebase.

# Test: Search for the old variable names. Expect: No occurrences of old names.
rg --type go 'num|times|maxDuration'

Length of output: 4177

Script:

#!/bin/bash
# Find the specific lines in transport/client.go where the renaming was done
rg 'sessionNum|reconnectAttempts|maxReconnectInterval' transport/client.go

# Check for the old variable names in the context of transport/client.go to confirm if they were renamed
rg 'num|times|maxDuration' transport/client.go

Length of output: 931

)
max = c.number
interval = c.reconnectInterval
if interval == 0 {
interval = reconnectInterval
reconnectInterval := c.reconnectInterval
if reconnectInterval == 0 {
reconnectInterval = defaultReconnectInterval
}
maxReconnectAttempts := c.maxReconnectAttempts
if maxReconnectAttempts == 0 {
maxReconnectAttempts = defaultMaxReconnectAttempts
}
connPoolSize := c.number
for {
if c.IsClosed() {
log.Warnf("client{peer:%s} goroutine exit now.", c.addr)
break
}

num = c.sessionNum()
if max <= num || max < times {
//Exit when the number of connection pools is sufficient or the reconnection times exceeds the connections numbers.
sessionNum = c.sessionNum()
if connPoolSize <= sessionNum || maxReconnectAttempts < reconnectAttempts {
//exit reconnect when the number of connection pools is sufficient or the current reconnection attempts exceeds the max reconnection attempts.
break
}
c.connect()
times++
if times > maxTimes {
maxDuration = int64(maxTimes) * int64(interval)
reconnectAttempts++
if reconnectAttempts > maxBackOffTimes {
maxReconnectInterval = int64(maxBackOffTimes) * int64(reconnectInterval)
} else {
maxDuration = int64(times) * int64(interval)
maxReconnectInterval = int64(reconnectAttempts) * int64(reconnectInterval)
}
<-gxtime.After(time.Duration(maxDuration))
<-gxtime.After(time.Duration(maxReconnectInterval))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of reConnect function modifications.

The renaming of parameters and the restructuring of the reconnect logic are consistent with the PR's objectives. However, the logic for calculating maxReconnectInterval could be simplified, and there's an inconsistency in the renaming (maxTimes was supposed to be defaultMaxReconnectAttempts but is changed to maxBackOffTimes).

- maxBackOffTimes = 10
+ defaultMaxReconnectAttempts = 10

Additionally, consider simplifying the calculation of maxReconnectInterval to improve readability.

- if reconnectAttempts > maxBackOffTimes {
-     maxReconnectInterval = int64(maxBackOffTimes) * int64(reconnectInterval)
- } else {
-     maxReconnectInterval = int64(reconnectAttempts) * int64(reconnectInterval)
- }
+ maxReconnectInterval = int64(min(reconnectAttempts, maxBackOffTimes)) * int64(reconnectInterval)

}
}

Expand Down
1 change: 1 addition & 0 deletions transport/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func TestTCPClient(t *testing.T) {
WithServerAddress(addr.String()),
WithReconnectInterval(5e8),
WithConnectionNumber(1),
WithReconnectAttempts(10),
)
assert.NotNil(t, clt)
assert.True(t, clt.ID() > 0)
Expand Down
17 changes: 13 additions & 4 deletions transport/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ func WithServerTlsConfigBuilder(tlsConfigBuilder TlsConfigBuilder) ServerOption
type ClientOption func(*ClientOptions)

type ClientOptions struct {
addr string
number int
reconnectInterval int // reConnect Interval

addr string
number int
reconnectInterval int // reConnect Interval
maxReconnectAttempts int // max reconnect attempts
// tls
sslEnabled bool
tlsConfigBuilder TlsConfigBuilder
Expand Down Expand Up @@ -168,3 +168,12 @@ func WithClientTlsConfigBuilder(tlsConfigBuilder TlsConfigBuilder) ClientOption
o.tlsConfigBuilder = tlsConfigBuilder
}
}

// WithReconnectAttempts @maxReconnectAttempts is max reconnect attempts.
func WithReconnectAttempts(maxReconnectAttempts int) ClientOption {
return func(o *ClientOptions) {
if 0 < maxReconnectAttempts {
o.maxReconnectAttempts = maxReconnectAttempts
}
}
}
Comment on lines +172 to +179
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段代码并没有被使用，可以先不加或者加注释。否则被人调用之后发现没有效果反而觉得是个问题。

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段代码并没有被使用，可以先不加或者加注释。否则被人调用之后发现没有效果反而觉得是个问题。

这是给上面提供的接口。要不 @No-SilverBullet 你在代码示例里面调用下？

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11

Loading