Skip to content

Add support for Oracle as backend database #89

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.

Sign up for GitHub

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

Closed
posulliv wants to merge 1 commit into from
Closed

Add support for Oracle as backend database #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/resource-groups-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,12 @@ curl -X POST http://localhost:8080/trino/resourcegroup/delete/{INSERT_ID_HERE}

## Add a selector

To add a single selector, specify all relevant fields in the body. Resource
group id should not be specified since the database should autoincrement it.
To add a single selector, specify all relevant fields in the body.

```$xslt
curl -X POST http://localhost:8080/trino/selector/create \
-d '{
"resourceGroupId": 1, \
Copy link
Copy Markdown
Member

@mosabua mosabua Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems like a bad idea .. how would you know what number to use? An id should be created automatically. Also .. this change should be independent and separate

"priority": 1, \
"userRegex": "selector1", \
"sourceRegex": "resourcegroup1", \
Expand Down
7 changes: 7 additions & 0 deletions gateway-ha/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,13 @@
<artifactId>slf4j-api</artifactId>
</dependency>

<dependency>
<groupId>com.oracle.database.jdbc</groupId>
<artifactId>ojdbc10</artifactId>
<version>19.21.0.0</version>
<scope>runtime</scope>
</dependency>

<dependency>
<groupId>io.dropwizard</groupId>
<artifactId>dropwizard-views-freemarker</artifactId>
Expand Down
1 change: 0 additions & 1 deletion gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/ResourceGroups.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ public static List<ResourceGroupsDetail> upcast(List<ResourceGroups> resourceGro
*/
public static void create(ResourceGroups model, ResourceGroupsDetail resourceGroupDetail)
{
model.set(resourceGroupId, resourceGroupDetail.getResourceGroupId());
model.set(name, resourceGroupDetail.getName());

model.set(parent, resourceGroupDetail.getParent());
Expand Down
70 changes: 70 additions & 0 deletions gateway-ha/src/main/resources/gateway-ha-persistence-oracle.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
CREATE TABLE gateway_backend (
name VARCHAR(256) PRIMARY KEY,
routing_group VARCHAR (256),
backend_url VARCHAR (256),
external_url VARCHAR (256),
active NUMBER(1)
);

CREATE TABLE query_history (
query_id VARCHAR(256) PRIMARY KEY,
query_text VARCHAR (256),
created NUMBER,
backend_url VARCHAR (256),
user_name VARCHAR(256),
source VARCHAR(256)
);
CREATE INDEX query_history_created_idx ON query_history(created);

CREATE TABLE resource_groups (
resource_group_id NUMBER GENERATED ALWAYS as IDENTITY(START with 1 INCREMENT by 1),
Copy link
Copy Markdown
Contributor

@willmostly willmostly Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know Oracle very well, but would a LONG be a better choice than a NUMBER? Iiuc a NUMBER with unspecified scale supports real numbers as well as integers. Not that it probably matters much

Copy link
Copy Markdown
Contributor Author

@posulliv posulliv Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a LONG in oracle is not actually a numeric data type confusingly enough :) https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/Data-Types.html#GUID-F6309DF8-162F-48A4-9454-FEE59EC6644F

I would vote to use NUMBER as the data type for the identity column here.

Copy link
Copy Markdown
Contributor

@willmostly willmostly Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing, I'd agree

name VARCHAR(250) NOT NULL,
-- OPTIONAL POLICY CONTROLS
parent NUMBER,
jmx_export CHAR(1),
scheduling_policy VARCHAR(128),
scheduling_weight NUMBER,
-- REQUIRED QUOTAS
soft_memory_limit VARCHAR(128) NOT NULL,
max_queued INT NOT NULL,
hard_concurrency_limit NUMBER NOT NULL,
-- OPTIONAL QUOTAS
soft_concurrency_limit NUMBER,
soft_cpu_limit VARCHAR(128),
hard_cpu_limit VARCHAR(128),
environment VARCHAR(128),
PRIMARY KEY(resource_group_id),
FOREIGN KEY (parent) REFERENCES resource_groups (resource_group_id) ON DELETE CASCADE
);

CREATE TABLE selectors (
resource_group_id NUMBER NOT NULL,
priority NUMBER NOT NULL,
-- Regex fields -- these will be used as a regular expression pattern to
-- match against the field of the same name on queries
user_regex VARCHAR(512),
source_regex VARCHAR(512),
-- Selector fields -- these must match exactly.
query_type VARCHAR(512),
client_tags VARCHAR(512),
selector_resource_estimate VARCHAR(1024),
FOREIGN KEY (resource_group_id) REFERENCES resource_groups (resource_group_id) ON DELETE CASCADE
);

CREATE TABLE resource_groups_global_properties (
name VARCHAR(128) NOT NULL PRIMARY KEY,
value VARCHAR(512) NULL,
CHECK (name in ('cpu_quota_period'))
);

CREATE TABLE exact_match_source_selectors(
environment VARCHAR(256),
update_time TIMESTAMP NOT NULL,
-- Selector fields which must exactly match a query
source VARCHAR(512) NOT NULL,
query_type VARCHAR(512),
resource_group_id VARCHAR(256) NOT NULL,
PRIMARY KEY (environment, source, resource_group_id),
UNIQUE (source, environment, query_type, resource_group_id)
);

4 changes: 2 additions & 2 deletions gateway-ha/src/main/resources/gateway-ha-persistence.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ CREATE INDEX query_history_created_idx ON query_history(created);

CREATE TABLE IF NOT EXISTS resource_groups (
resource_group_id BIGINT NOT NULL AUTO_INCREMENT,
name VARCHAR(250) NOT NULL UNIQUE,
name VARCHAR(250) NOT NULL,
Copy link
Copy Markdown
Contributor

@willmostly willmostly Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing the MySQL sql for this PR?

Copy link
Copy Markdown
Contributor Author

@posulliv posulliv Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to match the schema in trino for consistency - https://github.com/trinodb/trino/blob/master/plugin/trino-resource-group-managers/src/main/resources/db/migration/mysql/V2__add_resource_groups.sql

Happy to revert if unique names need to be enforced for gateway.

Copy link
Copy Markdown
Contributor Author

@posulliv posulliv Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One use case for non-unique names I have had in the past with Trino is storing the same resource group definitions for different environments in the same database. This is possible since resource groups are determined by the environment column of the resource_groups table. So you could use a single database and have the same resource group definitions for multiple environments.

Copy link
Copy Markdown
Contributor

@willmostly willmostly Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Trino clusters require an environment name set, while the gateway could front backends with different environment names. So environment could be less meaningful in the gateway context. OTOH, this is defined by the user, and they can choose not to complicate things.

I guess being able to import a set of policies from Trino to the gateway probably wins out.


-- OPTIONAL POLICY CONTROLS
parent BIGINT NULL,
Expand Down Expand Up @@ -75,4 +75,4 @@ CREATE TABLE IF NOT EXISTS exact_match_source_selectors (

PRIMARY KEY (environment, source, query_type),
UNIQUE (source, environment, query_type, resource_group_id)
);
);
24 changes: 12 additions & 12 deletions gateway-ha/src/test/java/io/trino/gateway/ha/router/TestResourceGroupsManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void testReadResourceGroup()
List<ResourceGroupsDetail> resourceGroups = resourceGroupManager.readAllResourceGroups(null);
assertThat(resourceGroups).hasSize(1);

assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(0).getName()).isEqualTo("admin");
assertThat(resourceGroups.get(0).getHardConcurrencyLimit()).isEqualTo(20);
assertThat(resourceGroups.get(0).getMaxQueued()).isEqualTo(200);
Expand All @@ -83,7 +83,7 @@ public void testReadResourceGroup()
public void testUpdateResourceGroup()
{
ResourceGroupsDetail resourceGroup = new ResourceGroupsDetail();
resourceGroup.setResourceGroupId(0L);
resourceGroup.setResourceGroupId(1L);
resourceGroup.setName("admin");
resourceGroup.setHardConcurrencyLimit(50);
resourceGroup.setMaxQueued(50);
Expand All @@ -96,7 +96,7 @@ public void testUpdateResourceGroup()

/* Update resourceGroups that do not exist yet.
* In this case, new resourceGroups should be created. */
resourceGroup.setResourceGroupId(1L);
resourceGroup.setResourceGroupId(2L);
resourceGroup.setName("localization-eng");
resourceGroup.setHardConcurrencyLimit(50);
resourceGroup.setMaxQueued(70);
Expand All @@ -118,14 +118,14 @@ public void testUpdateResourceGroup()

assertThat(resourceGroups).hasSize(3); // updated 2 non-existing groups, so count should be 3

assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(0).getName()).isEqualTo("admin");
assertThat(resourceGroups.get(0).getHardConcurrencyLimit()).isEqualTo(50);
assertThat(resourceGroups.get(0).getMaxQueued()).isEqualTo(50);
assertThat(resourceGroups.get(0).getJmxExport()).isEqualTo(Boolean.FALSE);
assertThat(resourceGroups.get(0).getSoftMemoryLimit()).isEqualTo("20%");

assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(2L);
assertThat(resourceGroups.get(1).getName()).isEqualTo("localization-eng");
assertThat(resourceGroups.get(1).getHardConcurrencyLimit()).isEqualTo(50);
assertThat(resourceGroups.get(1).getMaxQueued()).isEqualTo(70);
Expand All @@ -141,15 +141,15 @@ public void testDeleteResourceGroup()
List<ResourceGroupsDetail> resourceGroups = resourceGroupManager.readAllResourceGroups(null);
assertThat(resourceGroups).hasSize(3);

assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(2L);
assertThat(resourceGroups.get(2).getResourceGroupId()).isEqualTo(3L);

resourceGroupManager.deleteResourceGroup(resourceGroups.get(1).getResourceGroupId(), null);
resourceGroups = resourceGroupManager.readAllResourceGroups(null);

assertThat(resourceGroups).hasSize(2);
assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(resourceGroups.get(0).getResourceGroupId()).isEqualTo(1L);
assertThat(resourceGroups.get(1).getResourceGroupId()).isEqualTo(3L);
}

Expand All @@ -158,7 +158,7 @@ public void testDeleteResourceGroup()
public void testCreateSelector()
{
SelectorsDetail selector = new SelectorsDetail();
selector.setResourceGroupId(0L);
selector.setResourceGroupId(1L);
selector.setPriority(0L);
selector.setUserRegex("data-platform-admin");
selector.setSourceRegex("admin");
Expand All @@ -178,7 +178,7 @@ public void testReadSelector()
List<SelectorsDetail> selectors = resourceGroupManager.readAllSelectors(null);

assertThat(selectors).hasSize(1);
assertThat(selectors.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(selectors.get(0).getResourceGroupId()).isEqualTo(1L);
assertThat(selectors.get(0).getPriority()).isEqualTo(0L);
assertThat(selectors.get(0).getUserRegex()).isEqualTo("data-platform-admin");
assertThat(selectors.get(0).getSourceRegex()).isEqualTo("admin");
Expand All @@ -193,7 +193,7 @@ public void testUpdateSelector()
{
SelectorsDetail selector = new SelectorsDetail();

selector.setResourceGroupId(0L);
selector.setResourceGroupId(1L);
selector.setPriority(0L);
selector.setUserRegex("data-platform-admin_updated");
selector.setSourceRegex("admin_updated");
Expand Down Expand Up @@ -246,7 +246,7 @@ public void testDeleteSelector()
{
List<SelectorsDetail> selectors = resourceGroupManager.readAllSelectors(null);
assertThat(selectors).hasSize(3);
assertThat(selectors.get(0).getResourceGroupId()).isEqualTo(0L);
assertThat(selectors.get(0).getResourceGroupId()).isEqualTo(1L);
resourceGroupManager.deleteSelector(selectors.get(0), null);
selectors = resourceGroupManager.readAllSelectors(null);

Expand Down
1 change: 0 additions & 1 deletion gateway-ha/src/test/java/io/trino/gateway/ha/router/TestSpecificDbResourceGroupsManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ private void createResourceGroup()
{
ResourceGroupsDetail resourceGroup = new ResourceGroupsDetail();

resourceGroup.setResourceGroupId(1L);
resourceGroup.setName("admin2");
resourceGroup.setHardConcurrencyLimit(20);
resourceGroup.setMaxQueued(200);
Expand Down