-
Notifications
You must be signed in to change notification settings - Fork 293
[Model Monitoring] Add TimescaleDB database auto-creation #9012
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
Conversation
Add automatic database creation for TimescaleDB connector. When `auto_create_database` is enabled (default), the connector generates a database name using `system_id` and creates it if it doesn't exist. Changes: - Add `tsdb.auto_create_database` config option (default: True) - Add `admin_dsn()` method to `DatastoreProfilePostgreSQL` - Add `dsn(database=...)` parameter override - Add `_determine_database_name()` to `TimescaleDBConnector` - Add `_create_db_if_not_exists()` to `TimescaleDBOperationsManager` - Add unit tests for database auto-creation functionality
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.
LGTM by and large, but had some comments regarding semantics, style, logs, a minor input sanitation concern, and at least one place where we might need to close the connection pool.
mlrun/model_monitoring/db/tsdb/timescaledb/timescaledb_connector.py
Outdated
Show resolved
Hide resolved
mlrun/model_monitoring/db/tsdb/timescaledb/timescaledb_connector.py
Outdated
Show resolved
Hide resolved
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_connector.py
Outdated
Show resolved
Hide resolved
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_connector.py
Outdated
Show resolved
Hide resolved
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_connector.py
Outdated
Show resolved
Hide resolved
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_connector.py
Outdated
Show resolved
Hide resolved
tests/model_monitoring/db/tsdb/timescaledb/test_timescaledb_connector.py
Outdated
Show resolved
Hide resolved
mlrun/model_monitoring/db/tsdb/timescaledb/timescaledb_operations.py
Outdated
Show resolved
Hide resolved
Changes based on PR mlrun#9012 review: 1. URL encoding consistency in DatastoreProfiles: - Add URL encoding for user/password in dsn() methods - Add URL decoding in from_dsn() methods (urlparse doesn't decode) - Fix affects PostgreSQL, TDEngine, and Redis profiles - Add comprehensive tests for URL encoding round-trips 2. Terminology improvements: - Replace "maintenance database" with "default postgres database" - Replace "target database" with "monitoring database" 3. TimescaleDBConnection improvements: - Add close() method for proper encapsulation - Update timescaledb_operations to use close() instead of _pool.close()
Changes based on PR mlrun#9012 review: 1. URL encoding consistency in DatastoreProfiles: - Add URL encoding for user/password in dsn() methods - Add URL decoding in from_dsn() methods (urlparse doesn't decode) - Fix affects PostgreSQL, TDEngine, and Redis profiles - Add comprehensive tests for URL encoding round-trips 2. Terminology improvements: - Replace "maintenance database" with "default postgres database" - Replace "target database" with "monitoring database" 3. TimescaleDBConnection improvements: - Add close() method for proper encapsulation - Update timescaledb_operations to use close() instead of _pool.close()
Changes based on PR mlrun#9012 review: 1. URL encoding consistency in DatastoreProfiles: - Add URL encoding for user/password in dsn() methods - Add URL decoding in from_dsn() methods (urlparse doesn't decode) - Fix affects PostgreSQL, TDEngine, and Redis profiles - Add comprehensive tests for URL encoding round-trips 2. Terminology improvements: - Replace "maintenance database" with "default postgres database" - Replace "target database" with "monitoring database" 3. TimescaleDBConnection improvements: - Add close() method for proper encapsulation - Update timescaledb_operations to use close() instead of _pool.close()
gtopper
left a comment
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.
Other than that, LGTM ✅
Changes based on PR mlrun#9012 review: 1. URL encoding consistency in DatastoreProfiles: - Add URL encoding for user/password in dsn() methods - Add URL decoding in from_dsn() methods (urlparse doesn't decode) - Fix affects PostgreSQL, TDEngine, and Redis profiles - Add comprehensive tests for URL encoding round-trips 2. Terminology improvements: - Replace "maintenance database" with "default postgres database" - Replace "target database" with "monitoring database" 3. TimescaleDBConnection improvements: - Add close() method for proper encapsulation - Update timescaledb_operations to use close() instead of _pool.close()
Changes based on PR mlrun#9012 review: 1. URL encoding consistency in DatastoreProfiles: - Add URL encoding for user/password in dsn() methods - Add URL decoding in from_dsn() methods (urlparse doesn't decode) - Fix affects PostgreSQL, TDEngine, and Redis profiles - Add comprehensive tests for URL encoding round-trips 2. Terminology improvements: - Replace "maintenance database" with "default postgres database" - Replace "target database" with "monitoring database" 3. TimescaleDBConnection improvements: - Add close() method for proper encapsulation - Update timescaledb_operations to use close() instead of _pool.close()
TITLE: [Model Monitoring] Add TimescaleDB database auto-creation
Summary
Add automatic database creation for TimescaleDB connector. When
auto_create_databaseis enabled (default), the connector generates a database name usingsystem_idand creates it if it does not exist.Changes Made
tsdb.auto_create_databaseconfig option tomlrun/config.py(default: True)admin_dsn()method toDatastoreProfilePostgreSQLfor connecting to postgres maintenance databasedsn(database=...)parameter to override database in DSN_determine_database_name()toTimescaleDBConnector_create_db_if_not_exists()toTimescaleDBOperationsManageradmin_dsn()anddsn()methods (2 tests)Testing
Checklist
Reference