fix: plugin grpc server retry method #1096
Conversation
|
friendly ping @elezar /PTAL thanks |
|
@cdesiniotis can you help this ? |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Since we're changing this anyway. Let's use the built-in types.
internal/plugin/server.go
Outdated
| lastCrashTime := time.Now() | ||
| restartCount := 0 | ||
| maxRestarts := 5 | ||
| crashTimeoutSeconds := float64(3600) // 1 hour in seconds |
There was a problem hiding this comment.
| crashTimeoutSeconds := float64(3600) // 1 hour in seconds | |
| crashTimeout := 1 * time.Hour |
internal/plugin/server.go
Outdated
|
|
||
| klog.Infof("GRPC server for '%s' crashed with error: %v", plugin.rm.Resource(), err) | ||
|
|
||
| timeSinceLastCrash := time.Since(lastCrashTime).Seconds() |
There was a problem hiding this comment.
| timeSinceLastCrash := time.Since(lastCrashTime).Seconds() | |
| timeSinceLastCrash := time.Since(lastCrashTime) |
internal/plugin/server.go
Outdated
| timeSinceLastCrash, | ||
| ) | ||
|
|
||
| if timeSinceLastCrash > crashTimeoutSeconds { |
There was a problem hiding this comment.
| if timeSinceLastCrash > crashTimeoutSeconds { | |
| if timeSinceLastCrash > crashTimeout { |
internal/plugin/server.go
Outdated
| plugin.rm.Resource(), | ||
| err, | ||
| restartCount+1, | ||
| timeSinceLastCrash, |
There was a problem hiding this comment.
You probably need:
| timeSinceLastCrash, | |
| timeSinceLastCrash.Seconds(), |
or need to adjust the format string.
internal/plugin/server.go
Outdated
| // add a small delay before restarting to prevent tight loops | ||
| retryDelay := 5 * time.Second | ||
| klog.Infof("Waiting for %v before attempting to restart GRPC server for '%s'", retryDelay, plugin.rm.Resource()) | ||
| time.Sleep(retryDelay) // Wait for 5 seconds before attempting to restart |
There was a problem hiding this comment.
| time.Sleep(retryDelay) // Wait for 5 seconds before attempting to restart | |
| time.Sleep(retryDelay) |
| restartCount++ | ||
| } | ||
|
|
||
| // add a small delay before restarting to prevent tight loops |
There was a problem hiding this comment.
Is there a reason that you chose 5 seconds specifically? This seems quite long.
There was a problem hiding this comment.
Indeed, 5 seconds is too long, I changed it to 500 Millisecond. wdyt 🤔
internal/plugin/server.go
Outdated
| // it has been one hour since the last crash.. reset the count | ||
| // to reflect on the frequency |
There was a problem hiding this comment.
Let's rather just log the crashTimeout instead of a comment that is now further from the place where the one hour is specified.
internal/plugin/server.go
Outdated
| // quite if it has been restarted too often | ||
| // i.e. if server has crashed more than 5 times and it didn't last more than one hour each time |
There was a problem hiding this comment.
This comment should be moved to the docstring or at the point where we declare the variables.
Signed-off-by: googs1025 <googs1025@gmail.com>
|
friendly ping @elezar :) /PTAL |
2 participants
To improve observability and facilitate debugging, this PR enhances logging after the GRPC server crashes