feat: Add blocking timeout in polling config manager. #236
Conversation
…Availability using getConfig.
… figure out the reason.
|
converage is decreased, @yasirfolio3 let's discuss how coverage can be increased. |
|
|
||
| start := time.Now() | ||
| expectedExecutionTime := start.Add(blockingTimeout) | ||
| _, _ = configManager.GetConfig() |
There was a problem hiding this comment.
please add comment, blocking api for specified time.
pkg/config/polling_manager.go
Outdated
| @@ -36,6 +36,9 @@ import ( | |||
| // DefaultPollingInterval sets default interval for polling manager | |||
| const DefaultPollingInterval = 5 * time.Minute // default to 5 minutes for polling | |||
|
|
|||
| // DefaultBlockingTimeout sets timeout to block the config call until config has been initialized | |||
There was a problem hiding this comment.
| // DefaultBlockingTimeout sets timeout to block the config call until config has been initialized | |
| // DefaultBlockingTimeout is the default timeout for blocking in the GetConfig method until a config has been initialized |
pkg/utils/channel_utils.go
Outdated
| import "time" | ||
|
|
||
| // IsChannelClosed will check if the given channel is closed | ||
| func IsChannelClosed(ch chan struct{}) bool { |
There was a problem hiding this comment.
Nit: can we use read-only channel type <-chan struct{}? And also below in WaitForChannelToCloseOrTimeout?
pkg/config/polling_manager_test.go
Outdated
| func TestGetConfigForPollingConfigManagerWaitsForTimeoutForInvalidDatafile(t *testing.T) { | ||
| blockingTimeout := 50 * time.Millisecond | ||
| mockDatafile := []byte("NOT-VALID") | ||
| mockRequester := new(MockRequester) | ||
| mockRequester.On("Get", []utils.Header(nil)).Return(mockDatafile, http.Header{}, http.StatusOK, nil) | ||
|
|
||
| sdkKey := "test_sdk_key" | ||
|
|
||
| configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester), WithBlockingTimeout(blockingTimeout)) | ||
| mockRequester.AssertExpectations(t) | ||
|
|
||
| start := time.Now() | ||
| expectedExecutionTime := start.Add(blockingTimeout) | ||
| _, _ = configManager.GetConfig() | ||
| actualExecutionTime := start.Add(time.Since(start)) | ||
| assert.WithinDuration(t, expectedExecutionTime, actualExecutionTime, 10*time.Millisecond) | ||
| } |
There was a problem hiding this comment.
This test case and the others with 50ms blocking timeout seem to have the potential to be flaky, and will run very slowly. I will think about other ways to test this behavior - @pawels-optimizely @mikeng13 @mikecdavis any ideas?
correct, thanks for the correction. |
| // blocking api for specified time. | ||
| _, _ = configManager.GetConfig() | ||
| actualExecutionTime := start.Add(time.Since(start)) | ||
| assert.WithinDuration(t, expectedExecutionTime, actualExecutionTime, 10*time.Millisecond) |
There was a problem hiding this comment.
@yasirfolio3 @msohailhussain Following up on my earlier comment.
To limit flakiness I suggest we remove this assert.WithinDuration. Instead, we can implement a timeout on the total execution time of this test case, failing when it exceeds a pretty large duration, like 1-2 seconds. This lets us be resilient to varying execution time of GetConfig, while still ensuring that our test fails when the implementation of the timeout inside GetConfig is broken.
| @@ -146,6 +150,129 @@ func TestNewAsyncPollingProjectConfigManagerWithNullDatafile(t *testing.T) { | |||
| mockRequester.AssertExpectations(t) | |||
| } | |||
|
|
|||
| func TestGetConfigForPollingConfigManagerWaitsForTimeoutForInvalidDatafile(t *testing.T) { | |||
| blockingTimeout := 50 * time.Millisecond | |||
There was a problem hiding this comment.
I suggest reducing this to 1 millisecond, combined with my other suggestion, there is no need for this to be so high.
Summary
This introduces blocking timeout in PollingConfigManager. Config Manager will block any calling go-routine until the config is available or the timeout occurs.