Skip to content
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

Set up database connection pool #234

Merged
merged 2 commits into from Aug 29, 2019

Conversation

@hstonec
Copy link
Collaborator

commented Aug 19, 2019

This change is Reviewable

@googlebot googlebot added the cla: yes label Aug 19, 2019

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from e219b11 to d63dfd5 Aug 19, 2019

@hstonec hstonec requested a review from CydeWeys Aug 19, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @CydeWeys and @hstonec)


core/src/main/java/google/registry/config/files/default-config.yaml, line 205 at r1 (raw file):

  # "select table for update" statements directly.
  connectionIsolation: TRANSACTION_SERIALIZABLE
  # Whether to log all SQL queries. Overridable at runtime.

Logs them where? App Engine logs?


core/src/main/java/google/registry/config/files/default-config.yaml, line 206 at r1 (raw file):

  connectionIsolation: TRANSACTION_SERIALIZABLE
  # Whether to log all SQL queries. Overridable at runtime.
  showSql: false

Needs a better name. "logSqlQueries" would be more explanatory than "showSql".


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r1 (raw file):

  # Whether to log all SQL queries. Overridable at runtime.
  showSql: false
  # Never modify the schema. May use 'none' or 'validate'

OK, but what is this?


core/src/main/java/google/registry/config/files/default-config.yaml, line 209 at r1 (raw file):

  # Never modify the schema. May use 'none' or 'validate'
  hbm2ddlAuto: none
  # JDBC driver class name for PostgreSQL

What are the other options? If there are no other options, why bother making this a configuration option at all?


core/src/main/java/google/registry/config/files/default-config.yaml, line 213 at r1 (raw file):

  # Connection pool configurations.
  connectionProviderClass: org.hibernate.hikaricp.internal.HikariCPConnectionProvider

Ditto for this.


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

// limitations under the License.

package google.registry.model.transaction;

Why is this under model/, which we use for our Datastore entities, and not something specific to Cloud SQL? I know we were talking about /schema for the new entities themselves. Does this belong under there? Should we need a separate package for the Cloud SQL infrastructure classes like this?


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 34 at r1 (raw file):

/** Factory class to provide {@link EntityManagerFactory} instance. */
public class EntityManagerFactoryProvider {
  // This name must be same to the one defined in persistence.xml.

"must be the same as the one ..."

@CydeWeys CydeWeys requested a review from mindhog Aug 19, 2019

@CydeWeys
Copy link
Member

left a comment

+reviewer:@mindhog

Reviewable status: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)

@mindhog
Copy link
Member

left a comment

Reviewed 5 of 8 files at r1.
Reviewable status: 5 of 8 files reviewed, 10 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/build.gradle, line 224 at r1 (raw file):

  testCompile deps['org.seleniumhq.selenium:selenium-java']
  testCompile deps['org.seleniumhq.selenium:selenium-remote-driver']
  testCompile deps['org.testcontainers:postgresql']

FWIW, I added this in #230


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Why is this under model/, which we use for our Datastore entities, and not something specific to Cloud SQL? I know we were talking about /schema for the new entities themselves. Does this belong under there? Should we need a separate package for the Cloud SQL infrastructure classes like this?

FWIW, I started setting up hibernate classes under google.registry.hibernate but I don't feel strongly about the location. I think we probably want a package either under google.registry or google.registry.model, "hibernate" may not be a good choice because we may want it to contain code related to postgresql or sql dbs in general.


core/src/main/resources/META-INF/persistence.xml, line 22 at r1 (raw file):

      *  Use Hibernate's ServiceRegistry for bootstrapping (not JPA-compliant)
    -->
    <!-- <class>google.registry.schema.tmch.ClaimsList</class> -->

Should we add google.registry.model.domain.DomainBase?

I'd like to use this list in generate_sql_schema -- if you know how we can to that, feel free to add it to this CL. Otherwise I'll figure it out.


core/src/test/java/google/registry/model/transaction/EntityManagerFactoryProviderTest.java, line 39 at r1 (raw file):

  @Before
  public void init() {
    emf = EntityManagerFactoryProvider.create(

google-java-format likes it like this:

emf =
EntityManagerFactoryProvider.create(
database.getJdbcUrl(), database.getUsername(), database.getPassword());

You may want to run g-j-f directly on the new files.

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from d63dfd5 to eba521f Aug 20, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 8 files reviewed, 10 unresolved discussions (waiting on @CydeWeys and @mindhog)


core/build.gradle, line 224 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

FWIW, I added this in #230

Yeah, I just rebased on the master branch.


core/src/main/java/google/registry/config/files/default-config.yaml, line 205 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Logs them where? App Engine logs?

Yeah, because I can see the queries in the console if it is set to true.


core/src/main/java/google/registry/config/files/default-config.yaml, line 206 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Needs a better name. "logSqlQueries" would be more explanatory than "showSql".

Done.


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

OK, but what is this?

Added the explanation.


core/src/main/java/google/registry/config/files/default-config.yaml, line 209 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What are the other options? If there are no other options, why bother making this a configuration option at all?

Done.


core/src/main/java/google/registry/config/files/default-config.yaml, line 213 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Ditto for this.

Done.


core/src/main/resources/META-INF/persistence.xml, line 22 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

Should we add google.registry.model.domain.DomainBase?

I'd like to use this list in generate_sql_schema -- if you know how we can to that, feel free to add it to this CL. Otherwise I'll figure it out.

I just simply added DomainBase here, you may want to check if it works in your unit test.


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

FWIW, I started setting up hibernate classes under google.registry.hibernate but I don't feel strongly about the location. I think we probably want a package either under google.registry or google.registry.model, "hibernate" may not be a good choice because we may want it to contain code related to postgresql or sql dbs in general.

How about google.registry.db ?

We had a discussion here, and decided to put new Java entity classes under google.registry.schema but I don't feel comfortable to put non-schema stuff under it, thoughts?


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 34 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

"must be the same as the one ..."

Done.


core/src/test/java/google/registry/model/transaction/EntityManagerFactoryProviderTest.java, line 39 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

google-java-format likes it like this:

emf =
EntityManagerFactoryProvider.create(
database.getJdbcUrl(), database.getUsername(), database.getPassword());

You may want to run g-j-f directly on the new files.

Done.

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch 3 times, most recently from 4d8481f to e2c219a Aug 20, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 8 files reviewed, 10 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Added the explanation.

Based on this description, it sounds like we're never going to want to use it because it's not safe to do so, correct?

If so, may as well just remove it as a configuration option if we won't use it and won't support others who use it.


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

How about google.registry.db ?

We had a discussion here, and decided to put new Java entity classes under google.registry.schema but I don't feel comfortable to put non-schema stuff under it, thoughts?

If this is explicitly stuff related to hibernate, which it seems like it is, then google.registry.hibernate is probably suitable. It's less ambiguous than .db, which could be inferred to also include Datastore (which it won't).

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 8 files reviewed, 10 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

If this is explicitly stuff related to hibernate, which it seems like it is, then google.registry.hibernate is probably suitable. It's less ambiguous than .db, which could be inferred to also include Datastore (which it won't).

Or maybe google.registry.persistence instead? Technically, EntityManager(and its related stuff) is a JPA concept instead of Hibernate.

@mindhog
Copy link
Member

left a comment

Reviewed 1 of 8 files at r1, 7 of 7 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r2 (raw file):

  # Whether to log all SQL queries to App Engine logs. Overridable at runtime.
  logSqlQueries: false
  # Whether to automatically validates and exports schema DDL to the database when the

*validate


core/src/main/resources/META-INF/persistence.xml, line 22 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I just simply added DomainBase here, you may want to check if it works in your unit test.

If it doesn't break the tests, then it's at least harmless. I'll make a note to try it out.

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from e2c219a to 17d8049 Aug 26, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 7 of 8 files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r2 (raw file):

Previously, mindhog (Michael Muller) wrote…

*validate

Done.


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Or maybe google.registry.persistence instead? Technically, EntityManager(and its related stuff) is a JPA concept instead of Hibernate.

Bump this :)

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 7 of 8 files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @hstonec, and @mindhog)


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Bump this :)

Bump this again.

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from 17d8049 to 3fa63f7 Aug 27, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 5 of 8 files reviewed, 9 unresolved discussions (waiting on @CydeWeys and @mindhog)


core/src/main/java/google/registry/config/files/default-config.yaml, line 207 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Based on this description, it sounds like we're never going to want to use it because it's not safe to do so, correct?

If so, may as well just remove it as a configuration option if we won't use it and won't support others who use it.

Done.


core/src/main/java/google/registry/model/transaction/EntityManagerFactoryProvider.java, line 15 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Bump this again.

Discussed this with Ben offline, we decided to try google.registry.persistence first. If we feel there is better package name, we can always make a change later.

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @hstonec and @mindhog)


core/src/main/resources/META-INF/persistence.xml, line 22 at r3 (raw file):

      *  Use Hibernate's ServiceRegistry for bootstrapping (not JPA-compliant)
    -->
    <class>google.registry.model.domain.DomainBase</class>

Shouldn't this have ClaimsList?

(And not DomainBase maybe?)

@CydeWeys
Copy link
Member

left a comment

:lgtm:

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @hstonec and @mindhog)

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from 3fa63f7 to f5bbc62 Aug 28, 2019

@hstonec
Copy link
Collaborator Author

left a comment

Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @mindhog)


core/src/main/resources/META-INF/persistence.xml, line 22 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Shouldn't this have ClaimsList?

(And not DomainBase maybe?)

Added ClaimsList. DomainBase is for Michael's experiment.

@hstonec hstonec force-pushed the hstonec:add-conn-pool-pkg branch from f5bbc62 to c6321e5 Aug 29, 2019

Comments have been addressed.

@hstonec hstonec merged commit 487b695 into google:master Aug 29, 2019

4 of 7 checks passed

code-review/reviewable 5 files, 3 discussions left (CydeWeys, mindhog)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details

@hstonec hstonec deleted the hstonec:add-conn-pool-pkg branch Aug 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.