-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[IncomingQueryThrottler] Implement Incoming 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
base: main
Are you sure you want to change the base?
Changes from 6 commits
08fde38
80ba342
330d498
68a4fb2
99f54a9
50ce921
bfa35b6
8c8e381
f35abae
f97dc75
c3d3ac6
1747f1f
0d12e56
23194e2
30d6152
d44af58
0c322ab
14ffe15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package incomingquerythrottler | ||
|
||
import ( | ||
"context" | ||
|
||
querypb "vitess.io/vitess/go/vt/proto/query" | ||
topodatapb "vitess.io/vitess/go/vt/proto/topodata" | ||
) | ||
|
||
var _ ThrottlingStrategyHandler = (*CinnamonStrategy)(nil) | ||
|
||
// CinnamonStrategy is a placeholder for the Cinnamon throttling strategy. | ||
type CinnamonStrategy struct{} | ||
|
||
func (s *CinnamonStrategy) ThrottleIfNeeded(ctx context.Context, targetTabletType topodatapb.TabletType, sql string, transactionID int64, options *querypb.ExecuteOptions) error { | ||
// No-op for now | ||
return nil | ||
} | ||
|
||
// Start is a placeholder for initializing the Cinnamon throttling strategy. | ||
// TODO: Implement actual Cinnamon strategy initialization when the strategy is fully developed. | ||
func (s *CinnamonStrategy) Start() { | ||
// TODO: Initialize Cinnamon throttling strategy resources | ||
} | ||
|
||
// Stop is a placeholder for cleaning up the Cinnamon throttling strategy. | ||
// TODO: Implement actual Cinnamon strategy cleanup when the strategy is fully developed. | ||
func (s *CinnamonStrategy) Stop() { | ||
// TODO: Clean up Cinnamon throttling strategy resources | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
package incomingquerythrottler | ||
|
||
// ThrottlingStrategy represents the strategy used to apply throttling | ||
// to incoming queries based on system load or external signals. | ||
type ThrottlingStrategy string | ||
|
||
// Predefined throttling strategies for the IncomingQueryThrottler. | ||
const ( | ||
// ThrottlingStrategyTabletThrottler uses Vitess Tablet Throttler to shed load | ||
// from incoming queries when the tablet is under pressure. | ||
// Reference: https://vitess.io/docs/21.0/reference/features/tablet-throttler/ | ||
ThrottlingStrategyTabletThrottler ThrottlingStrategy = "TabletThrottler" | ||
|
||
// ThrottlingStrategyCinnamon uses Uber's Cinnamon load-shedding system | ||
// to regulate incoming queries under high load conditions. | ||
// Reference: https://www.uber.com/en-IN/blog/cinnamon-using-century-old-tech-to-build-a-mean-load-shedder/ | ||
ThrottlingStrategyCinnamon ThrottlingStrategy = "Cinnamon" | ||
|
||
// ThrottlingStrategyUnknown is used when the strategy is not known. | ||
ThrottlingStrategyUnknown ThrottlingStrategy = "Unknown" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please explain the need for strategies if the only two are the already existing TabletThrottler and the other is what looks like a catch-all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we chose to implement this using strategies to keep it extensible — for example, in our fork we’re already using Cinnamon in some places, and this structure makes it easier for others to plug in their own throttler strategies if they need to. Additionally, the strategy pattern provides clarity and modularity: each throttling approach is encapsulated and can evolve independently without complicating the core logic. This keeps the codebase easier to maintain and reason about, while also making it clear which strategy is being applied in a given context. |
||
|
||
// 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 ThrottlingStrategy `json:"strategy"` | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of this we can do factory based. where each implementation registers to the factory and then based on the config will load that strategy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done, this strategy is just a enum now. |
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) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package incomingquerythrottler | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"os" | ||
) | ||
|
||
var ( | ||
_ ConfigLoader = (*FileBasedConfigLoader)(nil) | ||
_osReadFile = os.ReadFile | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not update global state like this. Similar to below, we should pass in configuration like this and also then we don't need to mock / change That we add a dependency in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've incorporated the changes and used dependency injection for this |
||
_jsonUnmarshal = json.Unmarshal | ||
_configPath = "/config/throttler-config.json" | ||
) | ||
|
||
type FileBasedConfigLoader struct{} | ||
|
||
// NewFileBasedConfigLoader creates a new instance of FileBasedConfigLoader with the given file path. | ||
func NewFileBasedConfigLoader() *FileBasedConfigLoader { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
return &FileBasedConfigLoader{} | ||
} | ||
|
||
// Load reads the configuration from a file at the specific config path. | ||
func (f *FileBasedConfigLoader) Load(ctx context.Context) (Config, error) { | ||
data, err := _osReadFile(_configPath) | ||
if err != nil { | ||
return Config{}, err | ||
} | ||
|
||
var cfg Config | ||
if unMarshalErr := _jsonUnmarshal(data, &cfg); unMarshalErr != nil { | ||
return Config{}, unMarshalErr | ||
} | ||
|
||
return cfg, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,193 @@ | ||
package incomingquerythrottler | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"errors" | ||
"testing" | ||
|
||
"github.com/prashantv/gostub" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestNewFileBasedConfigLoader(t *testing.T) { | ||
loader := NewFileBasedConfigLoader() | ||
require.NotNil(t, loader) | ||
require.IsType(t, &FileBasedConfigLoader{}, loader) | ||
} | ||
|
||
func TestFileBasedConfigLoader_Load(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
stubOsReadFile func(filename string) ([]byte, error) | ||
stubJsonUnmarshal func(data []byte, v interface{}) error | ||
expectedConfig Config | ||
expectedError string | ||
expectedErrorNotNil bool | ||
}{ | ||
{ | ||
name: "successful config load with minimal config", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": true, "strategy": "TabletThrottler"}`), nil | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: true, | ||
Strategy: ThrottlingStrategyTabletThrottler, | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
{ | ||
name: "successful config load with disabled throttler", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": false, "strategy": "Unknown"}`), nil | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: false, | ||
Strategy: ThrottlingStrategyUnknown, | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
{ | ||
name: "file read error - file not found", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return nil, errors.New("no such file or directory") | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
// Should not be called | ||
t.Fatal("jsonUnmarshal should not be called when file read fails") | ||
return nil | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "no such file or directory", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "file read error - permission denied", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return nil, errors.New("permission denied") | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
// Should not be called | ||
t.Fatal("jsonUnmarshal should not be called when file read fails") | ||
return nil | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "permission denied", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "json unmarshal error - invalid json", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": true, "strategy": `), nil // incomplete JSON | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return errors.New("unexpected end of JSON input") | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "unexpected end of JSON input", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "json unmarshal error - invalid field type", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{"enabled": "invalid_boolean", "strategy": "TabletThrottler"}`), nil | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return errors.New("json: cannot unmarshal string into Go struct field Config.enabled of type bool") | ||
}, | ||
expectedConfig: Config{}, | ||
expectedError: "json: cannot unmarshal string into Go struct field Config.enabled of type bool", | ||
expectedErrorNotNil: true, | ||
}, | ||
{ | ||
name: "empty file - should unmarshal to zero value config", | ||
stubOsReadFile: func(filename string) ([]byte, error) { | ||
require.Equal(t, "/config/throttler-config.json", filename) | ||
return []byte(`{}`), nil | ||
}, | ||
stubJsonUnmarshal: func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
}, | ||
expectedConfig: Config{ | ||
Enabled: false, // zero value for bool | ||
Strategy: "", // zero value for ThrottlingStrategy | ||
}, | ||
expectedErrorNotNil: false, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Create stubs for the global variables | ||
osReadFileStub := gostub.Stub(&_osReadFile, tt.stubOsReadFile) | ||
jsonUnmarshalStub := gostub.Stub(&_jsonUnmarshal, tt.stubJsonUnmarshal) | ||
|
||
// Ensure stubs are reset after the test | ||
defer osReadFileStub.Reset() | ||
defer jsonUnmarshalStub.Reset() | ||
|
||
// Create loader and test Load method | ||
loader := NewFileBasedConfigLoader() | ||
config, err := loader.Load(context.Background()) | ||
|
||
// Verify error expectations | ||
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 correct config path is being used | ||
var capturedPath string | ||
|
||
stubOsReadFile := func(filename string) ([]byte, error) { | ||
capturedPath = filename | ||
return []byte(`{"enabled": true, "strategy": "TabletThrottler"}`), nil | ||
} | ||
|
||
stubJsonUnmarshal := func(data []byte, v interface{}) error { | ||
return json.Unmarshal(data, v) | ||
} | ||
|
||
// Create stubs | ||
osReadFileStub := gostub.Stub(&_osReadFile, stubOsReadFile) | ||
jsonUnmarshalStub := gostub.Stub(&_jsonUnmarshal, stubJsonUnmarshal) | ||
|
||
defer osReadFileStub.Reset() | ||
defer jsonUnmarshalStub.Reset() | ||
|
||
// Test | ||
loader := NewFileBasedConfigLoader() | ||
_, 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) | ||
} |
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.
why do we have this? There is no implementation.
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.
Removed it, the implementation was uber specific code that couldn't be open sourced.