Add PostgreSQL and Oracle migration files for DB backed resource group manager #9812

posulliv · 2021-10-29T01:14:05Z

I was unable to add an automated test for Oracle migrations. I wanted to use the image used in other tests wnameless/oracle-xe-11g-r2 but Flyway community edition will not run migrations with versions of Oracle that old. If someone knows of a newer Oracle image that can be used, I will be happy to add a test.

nineinchnick

I found this image: https://hub.docker.com/r/gvenzl/oracle-xe. Looks decent and it's used by the Testcontainers project.

...group-managers/src/main/resources/db/migration/postgresql/V1__add_resource_groups_schema.sql

posulliv · 2021-11-23T21:15:12Z

@nineinchnick thanks! I added a test using that Oracle image.

...rce-group-managers/src/main/resources/db/migration/oracle/V1__add_resource_groups_schema.sql

posulliv · 2021-12-09T15:27:04Z

@aalbu this is ready for another review whenever you get a chance. thanks!

...ers/src/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsFlywayMigration.java

posulliv · 2022-01-11T14:10:47Z

@aalbu thanks for the review! All suggestions have been applied.

hashhar

Why does Postgres have a single migration? Is it because DDL is transactional in Postgres? (hint: should be a code comment or in the commit message - I'd prefer to have separate migrations just to make it easier to compare across databases but existing is fine as well).

Looks good % comments.

Thanks for working on this @posulliv.

...resource-group-managers/src/main/java/io/trino/plugin/resourcegroups/db/FlywayMigration.java

...ers/src/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsFlywayMigration.java

hashhar · 2022-01-15T09:54:59Z

...rc/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsMysqlFlywayMigration.java

+    @Override
+    protected final JdbcDatabaseContainer<?> startContainer()
+    {
+        JdbcDatabaseContainer<?> container = new MySQLContainer<>("mysql:8.0.12");


@ebyhr @wendigo What do you think about starting to read image versions from a single properties file (or public constants) to ensure consistency in the versions being tested (easier to bump min and max tested versions that way).

@posulliv No change requested.

An alternative is to make versions injectable either from environment or system properties. That has the benefit that it's easier to test arbitrary versions via a matrix.

plugin/trino-resource-group-managers/pom.xml

...c/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsOracleFlywayMigration.java

posulliv · 2022-01-18T22:02:45Z

Why does Postgres have a single migration? Is it because DDL is transactional in Postgres? (hint: should be a code comment or in the commit message - I'd prefer to have separate migrations just to make it easier to compare across databases but existing is fine as well).

Yes, that's correct. I agree though that it would be more consistent to have a separate migration per table. So I updated the PostgreSQL migrations to be 1 file per table.

hashhar

Thanks for adding this @posulliv .

hashhar · 2022-01-19T06:23:18Z

Hmmm, seems like a test if failing now due to tests using H2.

Error:  io.trino.execution.resourcegroups.db.TestResourceGroupDbIntegration.testMemoryFraction  Time elapsed: 4.179 s  <<< FAILURE!
com.google.inject.CreationException: 
Unable to create injector, see the following errors:

1) Error notifying ProvisionListener LifeCycleModule$$Lambda$684/0x00000008004ee440 of DbResourceGroupConfig.
 Reason: LifeCycleStartException: Exception in PostConstruct method DbResourceGroupConfig::validate()
  at DbResourceGroupConfigurationManager.<init>(DbResourceGroupConfigurationManager.java:94)
      \_ for 2nd parameter config
  at H2ResourceGroupsModule.configure(H2ResourceGroupsModule.java:40)
  while locating DbResourceGroupConfigurationManager
  at H2ResourceGroupsModule.configure(H2ResourceGroupsModule.java:41)
  while locating ResourceGroupConfigurationManager
Caused by: LifeCycleStartException: Exception in PostConstruct method DbResourceGroupConfig::validate()
	at LifeCycleManager.startInstance(LifeCycleManager.java:246)
	at LifeCycleManager.addInstance(LifeCycleManager.java:212)
	at LifeCycleModule.provision(LifeCycleModule.java:62)
	at ProvisionListenerStackCallback$Provision.provision(ProvisionListenerStackCallback.java:117)
	at ProvisionListenerStackCallback.provision(ProvisionListenerStackCallback.java:66)
	at ProviderInternalFactory.circularGet(ProviderInternalFactory.java:62)
	at InternalFactoryToInitializableAdapter.get(InternalFactoryToInitializableAdapter.java:47)
	at SingleParameterInjector.inject(SingleParameterInjector.java:40)
	at SingleParameterInjector.getAll(SingleParameterInjector.java:60)
	at ConstructorInjector.provision(ConstructorInjector.java:113)
	at ConstructorInjector.access$000(ConstructorInjector.java:33)
	at ConstructorInjector$1.call(ConstructorInjector.java:98)
	at ProvisionListenerStackCallback$Provision.provision(ProvisionListenerStackCallback.java:109)
	at LifeCycleModule.provision(LifeCycleModule.java:54)
	at ProvisionListenerStackCallback$Provision.provision(ProvisionListenerStackCallback.java:117)
	at ProvisionListenerStackCallback.provision(ProvisionListenerStackCallback.java:66)
	at ConstructorInjector.construct(ConstructorInjector.java:93)
	at ConstructorBindingImpl$Factory.get(ConstructorBindingImpl.java:296)
	at ProviderToInternalFactoryAdapter.get(ProviderToInternalFactoryAdapter.java:40)
	at SingletonScope$1.get(SingletonScope.java:169)
	at InternalFactoryToProviderAdapter.get(InternalFactoryToProviderAdapter.java:45)
	at FactoryProxy.get(FactoryProxy.java:60)
	at ProviderToInternalFactoryAdapter.get(ProviderToInternalFactoryAdapter.java:40)
	at SingletonScope$1.get(SingletonScope.java:169)
	at InternalFactoryToProviderAdapter.get(InternalFactoryToProviderAdapter.java:45)
	at InternalInjectorCreator.loadEagerSingletons(InternalInjectorCreator.java:213)
	at InternalInjectorCreator.injectDynamically(InternalInjectorCreator.java:186)
	at InternalInjectorCreator.build(InternalInjectorCreator.java:113)
	at Guice.createInjector(Guice.java:87)
	at Bootstrap.initialize(Bootstrap.java:269)
	at H2ResourceGroupConfigurationManagerFactory.create(H2ResourceGroupConfigurationManagerFactory.java:62)
	at InternalResourceGroupManager.setConfigurationManager(InternalResourceGroupManager.java:163)
	at H2TestUtil.createQueryRunner(H2TestUtil.java:137)
	at H2TestUtil.createQueryRunner(H2TestUtil.java:122)
	at H2TestUtil.getSimpleQueryRunner(H2TestUtil.java:154)
	at TestResourceGroupDbIntegration.testMemoryFraction(TestResourceGroupDbIntegration.java:28)
	at java.base/NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/Method.invoke(Method.java:566)
	at MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at Invoker.invokeMethod(Invoker.java:645)
	at Invoker.invokeTestMethod(Invoker.java:851)
	at Invoker.invokeTestMethods(Invoker.java:1177)
	at TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129)
	at TestMethodWorker.run(TestMethodWorker.java:112)
	at java.base/ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/Thread.run(Thread.java:829)
Caused by: IllegalArgumentException: Invalid JDBC URL: jdbc:h2:mem:test_1295574013934079806. Only PostgreSQL, MySQL, and Oracle are supported.
	at DbResourceGroupConfig.validate(DbResourceGroupConfig.java:104)
	at java.base/NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/Method.invoke(Method.java:566)
	at LifeCycleManager.startInstance(LifeCycleManager.java:241)
	... 48 more

cc: @posulliv can you fix this? I'm fine with reverting the change suggested in #9812 (comment) and adding a comment why the validation is not done in the config class itself.

CI failed

posulliv · 2022-01-20T01:19:45Z

thanks @hashhar. I updated and added a comment.

I forgot the unit tests use the h2 database. Perhaps some future work could be to update those unit tests to use one of the supported database engines instead of h2 and then we can add the validation back to the config class itself.

hashhar · 2022-01-20T05:36:43Z

@aalbu / @nineinchnick Could you maybe take another look at the migrations being added since once added we won't get a chance to edit them - we can only add new migrations.

This looks good to me otherwise and ready to merge.

nineinchnick · 2022-01-20T08:58:50Z

@hashhar LGTM

hashhar · 2022-01-20T10:50:52Z

Thanks @posulliv for seeing this to the finish line.

Can you PTAL at suggested release note at #10552 (comment)? Feel free to suggest changes.

also we should add some docs to help users to use DB backed resource groups. I don't think there is anything existing already? cc: @mosabua do you know?

posulliv · 2022-01-20T14:38:21Z

@hashhar there is an open PR from a long time back to add docs for this - #4523

Needs to be updated now. I will work on updating that docs PR.

Release note comment looks good to me. Thanks!!

mosabua · 2022-01-20T18:57:11Z

Awesome @posulliv .. ping me when you want some input for the docs PR and anything related.

cla-bot bot added the cla-signed label Oct 29, 2021

posulliv requested review from electrum and findepi November 17, 2021 22:36

findepi requested review from nineinchnick and aalbu November 22, 2021 15:38

nineinchnick approved these changes Nov 23, 2021

View reviewed changes

...group-managers/src/main/resources/db/migration/postgresql/V1__add_resource_groups_schema.sql Outdated Show resolved Hide resolved

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch from 54aeec5 to 8574806 Compare November 23, 2021 21:14

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch from 8574806 to 8e0b0ea Compare November 29, 2021 16:48

aalbu reviewed Nov 30, 2021

View reviewed changes

...rce-group-managers/src/main/resources/db/migration/oracle/V1__add_resource_groups_schema.sql Outdated Show resolved Hide resolved

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch 2 times, most recently from d41f5d4 to 2c646b3 Compare December 8, 2021 21:19

posulliv requested review from aalbu and hashhar December 14, 2021 02:53

aalbu approved these changes Dec 21, 2021

View reviewed changes

...ers/src/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsFlywayMigration.java Outdated Show resolved Hide resolved

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch from 2c646b3 to e0ee7f6 Compare January 10, 2022 20:37

hashhar mentioned this pull request Jan 13, 2022

Add primary key to selectors table #10586

Merged

hashhar reviewed Jan 15, 2022

View reviewed changes

hashhar assigned posulliv Jan 18, 2022

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch from e0ee7f6 to 13c7581 Compare January 18, 2022 22:01

hashhar previously approved these changes Jan 19, 2022

View reviewed changes

posulliv added 2 commits January 19, 2022 16:16

Add PostgreSQL migration files for DB resource groups

7011aa3

Add Oracle migration files for DB resource groups

4dc2c90

posulliv force-pushed the postgresql-oracle-resource-groups-migrations branch from 13c7581 to 4dc2c90 Compare January 19, 2022 21:28

hashhar approved these changes Jan 20, 2022

View reviewed changes

hashhar merged commit 23ca99c into trinodb:master Jan 20, 2022

github-actions bot added this to the 369 milestone Jan 20, 2022

hashhar mentioned this pull request Jan 20, 2022

Release notes for 369 #10552

Closed

posulliv deleted the postgresql-oracle-resource-groups-migrations branch January 20, 2022 14:37

This was referenced Jan 20, 2022

Add Trino 369 release notes #10553

Merged

Document DB backed resource group manager #10742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PostgreSQL and Oracle migration files for DB backed resource group manager #9812

Add PostgreSQL and Oracle migration files for DB backed resource group manager #9812

posulliv commented Oct 29, 2021

nineinchnick left a comment

posulliv commented Nov 23, 2021

posulliv commented Dec 9, 2021

posulliv commented Jan 11, 2022

hashhar left a comment •

edited

Loading

hashhar Jan 15, 2022

hashhar Jan 15, 2022

posulliv commented Jan 18, 2022

hashhar left a comment

hashhar commented Jan 19, 2022

posulliv commented Jan 20, 2022

hashhar commented Jan 20, 2022

nineinchnick commented Jan 20, 2022

hashhar commented Jan 20, 2022 •

edited

Loading

posulliv commented Jan 20, 2022

mosabua commented Jan 20, 2022

Add PostgreSQL and Oracle migration files for DB backed resource group manager #9812

Add PostgreSQL and Oracle migration files for DB backed resource group manager #9812

Conversation

posulliv commented Oct 29, 2021

nineinchnick left a comment

Choose a reason for hiding this comment

posulliv commented Nov 23, 2021

posulliv commented Dec 9, 2021

posulliv commented Jan 11, 2022

hashhar left a comment • edited Loading

Choose a reason for hiding this comment

hashhar Jan 15, 2022

Choose a reason for hiding this comment

hashhar Jan 15, 2022

Choose a reason for hiding this comment

posulliv commented Jan 18, 2022

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Jan 19, 2022

posulliv commented Jan 20, 2022

hashhar commented Jan 20, 2022

nineinchnick commented Jan 20, 2022

hashhar commented Jan 20, 2022 • edited Loading

posulliv commented Jan 20, 2022

mosabua commented Jan 20, 2022

hashhar left a comment •

edited

Loading

hashhar commented Jan 20, 2022 •

edited

Loading