-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[QueryThrottler] Implement Query Throttler with Strategy Pattern #18449
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.
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
Open
siddharth16396
wants to merge
24
commits into
vitessio:main
Choose a base branch
from
siddharth16396:throttler-first
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
08fde38
[IncomingQueryThrottler] Add foundational interfaces for incoming que…
siddharth16396 80ba342
incomingquerythrottler: implement FileBasedConfigLoader
siddharth16396 330d498
Re incomingquerythrottler: implement FileBasedConfigLoader
siddharth16396 68a4fb2
IncomingQueryThrottler: Add NOOP strategy
siddharth16396 99f54a9
feat: implement incoming query throttler with strategy pattern
siddharth16396 50ce921
minor lints
siddharth16396 bfa35b6
Use Dependency Injection instead of stubs in FileBasedConfigLoader
siddharth16396 8c8e381
remove cinnamon strategy references
siddharth16396 f35abae
Use tablet env flag instead of static refresher ticker variable
siddharth16396 f97dc75
Refactor IQT interface and create registry package
siddharth16396 c3d3ac6
Linting
siddharth16396 1747f1f
Minor refactor between iqt/registry
siddharth16396 0d12e56
Minor optimizations in IQT
siddharth16396 23194e2
Adding UTs for registry.go
siddharth16396 30d6152
Use require.Eventually() instead of sleeping in tests
siddharth16396 d44af58
call incoming query throttler in tablet server
siddharth16396 0c322ab
minor lints
siddharth16396 14ffe15
Merge remote-tracking branch 'upstream/main' into throttler-first
siddharth16396 89f9719
Fix the issue of e2e failing bcoz of connection pool name panic
siddharth16396 57fd279
Add close call of IQT to state manager
siddharth16396 051d37d
Attempt to fixing e2e failing due to flags added
siddharth16396 bfa8885
Fix data-race condition in config refresh loop
siddharth16396 5f37a1d
Fix another uncaught but possible data race in tests
siddharth16396 c326643
rename package and references to "query-throttler"
siddharth16396 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
21 changes: 21 additions & 0 deletions
21
go/vt/vttablet/tabletserver/incomingquerythrottler/config.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package incomingquerythrottler | ||
|
||
import "vitess.io/vitess/go/vt/vttablet/tabletserver/incomingquerythrottler/registry" | ||
|
||
// Compile-time interface compliance check | ||
var _ registry.StrategyConfig = (*Config)(nil) | ||
|
||
// Config defines the runtime configuration for the IncomingQueryThrottler. | ||
// It specifies whether throttling is enabled and which strategy to use. | ||
type Config struct { | ||
// Enabled indicates whether the throttler should actively apply throttling logic. | ||
Enabled bool `json:"enabled"` | ||
|
||
// Strategy selects which throttling strategy should be used. | ||
Strategy registry.ThrottlingStrategy `json:"strategy"` | ||
} | ||
|
||
// GetStrategy implements registry.StrategyConfig interface | ||
func (c Config) GetStrategy() registry.ThrottlingStrategy { | ||
return c.Strategy | ||
} |
8 changes: 8 additions & 0 deletions
8
go/vt/vttablet/tabletserver/incomingquerythrottler/config_loader_interface.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package incomingquerythrottler | ||
|
||
import "context" | ||
|
||
type ConfigLoader interface { | ||
// Load returns the latest throttler config (may come from file, topo, etc.) | ||
Load(ctx context.Context) (Config, error) | ||
} |
53 changes: 53 additions & 0 deletions
53
go/vt/vttablet/tabletserver/incomingquerythrottler/file_based_config_loader.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
package incomingquerythrottler | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"os" | ||
) | ||
|
||
const defaultConfigPath = "/config/throttler-config.json" | ||
|
||
var _ ConfigLoader = (*FileBasedConfigLoader)(nil) | ||
|
||
// FileBasedConfigLoader implements ConfigLoader by reading configuration from a JSON file. | ||
type FileBasedConfigLoader struct { | ||
configPath string | ||
readFile func(string) ([]byte, error) | ||
unmarshal func([]byte, interface{}) error | ||
} | ||
|
||
// NewFileBasedConfigLoader creates a new instance of FileBasedConfigLoader. | ||
// It uses the standard config path "/config/throttler-config.json" and standard os.ReadFile and json.Unmarshal functions. | ||
func NewFileBasedConfigLoader() *FileBasedConfigLoader { | ||
return &FileBasedConfigLoader{ | ||
configPath: defaultConfigPath, | ||
readFile: os.ReadFile, | ||
unmarshal: json.Unmarshal, | ||
} | ||
} | ||
|
||
// NewFileBasedConfigLoaderWithDeps creates a new instance with custom dependencies for testing. | ||
// This allows injection of mock functions without global state modification. | ||
func NewFileBasedConfigLoaderWithDeps(configPath string, readFile func(string) ([]byte, error), unmarshal func([]byte, interface{}) error) *FileBasedConfigLoader { | ||
return &FileBasedConfigLoader{ | ||
configPath: configPath, | ||
readFile: readFile, | ||
unmarshal: unmarshal, | ||
} | ||
} | ||
|
||
// Load reads the configuration from the configured file path. | ||
func (f *FileBasedConfigLoader) Load(ctx context.Context) (Config, error) { | ||
data, err := f.readFile(f.configPath) | ||
siddharth16396 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
return Config{}, err | ||
} | ||
|
||
var cfg Config | ||
if unMarshalErr := f.unmarshal(data, &cfg); unMarshalErr != nil { | ||
return Config{}, unMarshalErr | ||
} | ||
|
||
return cfg, nil | ||
} |
204 changes: 204 additions & 0 deletions
204
go/vt/vttablet/tabletserver/incomingquerythrottler/file_based_config_loader_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
package incomingquerythrottler | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"errors" | ||
"testing" | ||
|
||
"vitess.io/vitess/go/vt/vttablet/tabletserver/incomingquerythrottler/registry" | ||
|
||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestNewFileBasedConfigLoader(t *testing.T) { | ||
loader := NewFileBasedConfigLoader() | ||
require.NotNil(t, loader) | ||
require.IsType(t, &FileBasedConfigLoader{}, loader) | ||
require.Equal(t, defaultConfigPath, loader.configPath) | ||
} | ||
|
||
func TestFileBasedConfigLoader_Load(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
configPath string | ||
mockReadFile func(filename string) ([]byte, error) | ||
mockJsonUnmarshal func(data []byte, v interface{}) error | ||
expectedConfig Config | ||
expectedError string | ||
expectedErrorNotNil bool | ||
}{ | ||
{ | ||
name: "successful config load with minimal config", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": true, "strategy": "TabletThrottler"}`), nil | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: true, | ||
Strategy: registry.ThrottlingStrategyTabletThrottler, | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
{ | ||
name: "successful config load with disabled throttler", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": false, "strategy": "TabletThrottler"}`), nil | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: false, | ||
Strategy: registry.ThrottlingStrategyTabletThrottler, | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
{ | ||
name: "file read error - file not found", | ||
configPath: "/nonexistent/config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/nonexistent/config.json", filename) | ||
return nil, errors.New("no such file or directory") | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "no such file or directory", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "file read error - permission denied", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return nil, errors.New("permission denied") | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "permission denied", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "json unmarshal error - invalid json", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": true`), nil | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "unexpected end of JSON input", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "json unmarshal error - invalid field type", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": "not_a_boolean", "strategy": "TabletThrottler"}`), nil | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "cannot unmarshal string into Go struct field Config.enabled of type bool", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "empty file - should unmarshal to zero value config", | ||
configPath: "/config/throttler-config.json", | ||
mockReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{}`), nil | ||
}, | ||
mockJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: false, | ||
Strategy: "", | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Create loader with injected dependencies | ||
loader := NewFileBasedConfigLoaderWithDeps(tt.configPath, tt.mockReadFile, tt.mockJsonUnmarshal) | ||
|
||
// Test | ||
config, err := loader.Load(context.Background()) | ||
|
||
// Assert | ||
if tt.expectedErrorNotNil { | ||
require.Error(t, err) | ||
require.Contains(t, err.Error(), tt.expectedError) | ||
require.Equal(t, tt.expectedConfig, config) | ||
} else { | ||
require.NoError(t, err) | ||
require.Equal(t, tt.expectedConfig, config) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestFileBasedConfigLoader_Load_ConfigPath(t *testing.T) { | ||
// Test that the production loader uses the default config path | ||
var capturedPath string | ||
|
||
mockReadFile := func(filename string) ([]byte, error) { | ||
capturedPath = filename | ||
return []byte(`{"enabled": true, "strategy": "TabletThrottler"}`), nil | ||
} | ||
|
||
mockJsonUnmarshal := func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
} | ||
|
||
// Test with production constructor (should use default path) | ||
loader := NewFileBasedConfigLoaderWithDeps(defaultConfigPath, mockReadFile, mockJsonUnmarshal) | ||
_, err := loader.Load(context.Background()) | ||
|
||
require.NoError(t, err) | ||
require.Equal(t, "/config/throttler-config.json", capturedPath) | ||
} | ||
|
||
func TestFileBasedConfigLoader_ImplementsConfigLoader(t *testing.T) { | ||
// Verify that FileBasedConfigLoader implements ConfigLoader interface | ||
var _ ConfigLoader = (*FileBasedConfigLoader)(nil) | ||
|
||
// This should compile without issues, proving interface compliance | ||
loader := NewFileBasedConfigLoader() | ||
require.NotNil(t, loader) | ||
} | ||
|
||
func TestNewFileBasedConfigLoaderWithDeps(t *testing.T) { | ||
configPath := "/test/config.json" | ||
mockReadFile := func(string) ([]byte, error) { return nil, nil } | ||
mockUnmarshal := func([]byte, interface{}) error { return nil } | ||
|
||
loader := NewFileBasedConfigLoaderWithDeps(configPath, mockReadFile, mockUnmarshal) | ||
|
||
require.NotNil(t, loader) | ||
require.Equal(t, configPath, loader.configPath) | ||
// Note: We can't directly test function equality, but the constructor should set them | ||
} | ||
|
||
func TestFileBasedConfigLoader_UsesDefaultPath(t *testing.T) { | ||
// Test that the production constructor uses the default path | ||
loader := NewFileBasedConfigLoader() | ||
require.Equal(t, "/config/throttler-config.json", loader.configPath) | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a TopoServerConfig loader as well, but that will be a seperate PR by another team mate