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

Add initial support for persisting premium lists to Cloud SQL #285

Merged
merged 12 commits into from Oct 8, 2019

Conversation

@CydeWeys
Copy link
Member

commented Sep 25, 2019

This adds support to the nomulus create_premium_list command only; support for
nomulus update_premium_list will be in a subsequent PR.

The design goals for this PR were:

  1. Do not change the existing codepaths for premium lists at all, especially not
    on the read path.
  2. Write premium lists to Cloud SQL only if requested (i.e. not by default), and
    write to Datastore first so as to not be blocked by errors with Cloud SQL.
  3. Reuse existing codepaths to the maximum possible extent (e.g. don't yet
    re-implement premium list parsing; take advantage of the existing logic), but
    also ...
  4. Some duplication is OK, since the existing Datastore path will be deleted
    once this migration is complete, leaving only the codepaths for Cloud SQL.

This change is Reviewable

@googlebot googlebot added the cla: yes label Sep 25, 2019
@CydeWeys CydeWeys requested review from gbrodman and hstonec Sep 25, 2019
Copy link
Member Author

left a comment

+reviewer:@gbrodman +reviewer:@hstonec

This is an initial WIP PR to validate the migration approach taken. I'll be working on tests next.

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @gbrodman and @hstonec)

Copy link
Collaborator

left a comment

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java, line 85 at r1 (raw file):

  @Override
  protected void saveToCloudSql() {

Thoughts on wrapping these types of actions in a DAO? It might make it significantly easier to encapsulate all the DB reads/writes in one place

Copy link
Collaborator

left a comment

Reviewed 5 of 6 files at r1.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @gbrodman and @hstonec)


core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java, line 85 at r1 (raw file):

Previously, gbrodman wrote…

Thoughts on wrapping these types of actions in a DAO? It might make it significantly easier to encapsulate all the DB reads/writes in one place

+1, we may not want to expose SQL/database schema in such Action class. Also, is it possible to refactor this method and saveToDatastore to extract a method for the common logic?

Copy link
Member Author

left a comment

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java, line 85 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

+1, we may not want to expose SQL/database schema in such Action class. Also, is it possible to refactor this method and saveToDatastore to extract a method for the common logic?

The reason I'm not extracting a method for common logic is that dual-writing is a temporary state of affairs. The saveToDatastore() method is going to be deleted in short order, so it doesn't make sense to do additional work to refactor out the common functionality (and also adding increased risk in modifying the currently used code path), only to then un-factor it out a few weeks down the line when it's back to only being called from one place.

Working on the DAO now.

Copy link
Member Author

left a comment

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @gbrodman and @hstonec)


core/src/main/java/google/registry/tools/server/CreatePremiumListAction.java, line 85 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The reason I'm not extracting a method for common logic is that dual-writing is a temporary state of affairs. The saveToDatastore() method is going to be deleted in short order, so it doesn't make sense to do additional work to refactor out the common functionality (and also adding increased risk in modifying the currently used code path), only to then un-factor it out a few weeks down the line when it's back to only being called from one place.

Working on the DAO now.

PTAL, now with DAO and a factored out utility method.

Copy link
Member Author

left a comment

Note that this PR is now dependent on the existence of PR #282

Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @gbrodman and @hstonec)

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from 4671389 to 8de537f Sep 27, 2019
Copy link
Collaborator

left a comment

Reviewed 4 of 5 files at r2.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/schema/tld/PremiumListDao.java, line 31 at r2 (raw file):

Quoted 4 lines of code…
              checkArgument(
                  !checkExists(premiumList.getName()),
                  "A premium list of this name already exists: %s.",
                  premiumList.getName());

Does this have to be in the transaction?


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 90 at r2 (raw file):

  }

  google.registry.schema.tld.PremiumList parseInputToPremiumList() {

Maybe add a unit test for this?

Copy link
Collaborator

left a comment

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/schema/tld/PremiumListDao.java, line 31 at r2 (raw file):

Previously, hstonec (Shicong Huang) wrote…
              checkArgument(
                  !checkExists(premiumList.getName()),
                  "A premium list of this name already exists: %s.",
                  premiumList.getName());

Does this have to be in the transaction?

Oh, never mind, I didn't notice the call to checkExists().

Copy link
Collaborator

left a comment

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 33 at r2 (raw file):

/** Unit tests for {@link PremiumListDao}. */
@RunWith(JUnit4.class)
public class PremiumListDaoTest {

Add a test to try to save an existing premium list?

Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/schema/tld/PremiumListDao.java, line 31 at r2 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Oh, never mind, I didn't notice the call to checkExists().

Yeah, it has to be in the transaction to solve a potential race condition wherein two commands are running simultaneously that both create the same premium list.

Copy link
Member Author

left a comment

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


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 90 at r2 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Maybe add a unit test for this?

Done.

Copy link
Collaborator

left a comment

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


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 47 at r3 (raw file):

@Table(
    name = "PremiumList",
    indexes = {@Index(columnList = "name", name = "name_idx")})

I think the indexes are supposed to be universally unique so it might behoove us to use a more specific index name here


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 58 at r3 (raw file):

  private Long revisionId;

  @Column(name = "creation_timestamp", nullable = false)

This may not be in the scope of this PR directly, but we can remove the names on these elements now that we have the naming strategy


core/src/main/java/google/registry/schema/tld/PremiumListDao.java, line 24 at r3 (raw file):

  /** Persist a new premium list to Cloud SQL. */
  public static void saveNew(PremiumList premiumList) {

Are you thinking that there will be a separate method for updating? Do we know what the signature of that method will be?

Copy link
Collaborator

left a comment

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


core/src/test/java/google/registry/tools/server/CreateOrUpdatePremiumListActionTest.java, line 64 at r3 (raw file):

    assertThat(premiumList.getName()).isEqualTo("testlist");
    assertThat(premiumList.getLabelsToPrices())
        .containsExactlyEntriesIn(

can just do containsExactly right?

Copy link
Member Author

left a comment

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


core/src/main/java/google/registry/schema/tld/PremiumListDao.java, line 24 at r3 (raw file):

Previously, gbrodman wrote…

Are you thinking that there will be a separate method for updating? Do we know what the signature of that method will be?

Yes, there will be a separate method, to come in the subsequent PR that adds support for the UpdatePremiumListAction.

However, note that it is still saving a new row to the DB (the revision ID is increasing). The only difference there is it won't have the check that the premium list doesn't already exist. The method signature will be the same.


core/src/test/java/google/registry/tools/server/CreateOrUpdatePremiumListActionTest.java, line 64 at r3 (raw file):

Previously, gbrodman wrote…

can just do containsExactly right?

Done.

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from 02166e5 to 984c16b Oct 4, 2019
Copy link
Member Author

left a comment

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


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 47 at r3 (raw file):

Previously, gbrodman wrote…

I think the indexes are supposed to be universally unique so it might behoove us to use a more specific index name here

Done.


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 58 at r3 (raw file):

Previously, gbrodman wrote…

This may not be in the scope of this PR directly, but we can remove the names on these elements now that we have the naming strategy

Done.


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 33 at r2 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Add a test to try to save an existing premium list?

Done.

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from 1fbd017 to 3d79902 Oct 4, 2019
Copy link
Collaborator

left a comment

Reviewable status: 4 of 9 files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 46 at r4 (raw file):

@Entity
@Table(
    name = "PremiumList",

Sorry for missing this before, but we shouldn't need the name field here -- it defaults to the entity name so that'll already be PremiumList here

(some of the test classes use this annotation because otherwise their name would be like FooTest.TestEntity instead of just TestEntity)

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from 3d79902 to 377295d Oct 7, 2019
Copy link
Member Author

left a comment

Reviewable status: 4 of 9 files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 58 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Done.

I get the following error when I remove name from the revisionId column and run nomulus generate_sql_schema. It seems like something still isn't working quite right:

Caused by: org.hibernate.cfg.RecoverableException: Unable to find column with logical name: revision_id in org.hibernate.mapping.Table(PremiumList) and its related supertables and secondary tables
at org.hibernate.cfg.Ejb3JoinColumn.checkReferencedColumnsType(Ejb3JoinColumn.java:833)
at org.hibernate.cfg.BinderHelper.createSyntheticPropertyReference(BinderHelper.java:256)
at org.hibernate.cfg.annotations.CollectionBinder.bindCollectionSecondPass(CollectionBinder.java:1693)
... 15 more
Caused by: org.hibernate.MappingException: Unable to find column with logical name: revision_id in org.hibernate.mapping.Table(PremiumList) and its related supertables and secondary tables
at org.hibernate.cfg.Ejb3JoinColumn.checkReferencedColumnsType(Ejb3JoinColumn.java:828)
... 17 more

Copy link
Collaborator

left a comment

Reviewable status: 4 of 9 files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @hstonec)


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 58 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I get the following error when I remove name from the revisionId column and run nomulus generate_sql_schema. It seems like something still isn't working quite right:

Caused by: org.hibernate.cfg.RecoverableException: Unable to find column with logical name: revision_id in org.hibernate.mapping.Table(PremiumList) and its related supertables and secondary tables
at org.hibernate.cfg.Ejb3JoinColumn.checkReferencedColumnsType(Ejb3JoinColumn.java:833)
at org.hibernate.cfg.BinderHelper.createSyntheticPropertyReference(BinderHelper.java:256)
at org.hibernate.cfg.annotations.CollectionBinder.bindCollectionSecondPass(CollectionBinder.java:1693)
... 15 more
Caused by: org.hibernate.MappingException: Unable to find column with logical name: revision_id in org.hibernate.mapping.Table(PremiumList) and its related supertables and secondary tables
at org.hibernate.cfg.Ejb3JoinColumn.checkReferencedColumnsType(Ejb3JoinColumn.java:828)
... 17 more

The annotations on labelsToPrices should reference the logical name of the column (revisionid) rather than the physical name of the column (revision_id). This is because the camelCase name only gets translated to the snake_case name essentially at the last step when it's written out to disk. I saw the same issue in my PR https://reviewable.io/reviews/google/nomulus/290#- at

@Index(name = "idx_registry_lock_verification_code", columnList = "verificationCode")

CydeWeys added 5 commits Sep 25, 2019
This adds support to the `nomulus create_premium_list` command only; support for
`nomulus update_premium_list` will be in a subsequent PR.

The design goals for this PR were:
1. Do not change the existing codepaths for premium lists at all, especially not
   on the read path.
2. Write premium lists to Cloud SQL only if requested (i.e. not by default), and
   write to Datastore first so as to not be blocked by errors with Cloud SQL.
3. Reuse existing codepaths to the maximum possible extent (e.g. don't yet
   re-implement premium list parsing; take advantage of the existing logic), but
   also ...
4. Some duplication is OK, since the existing Datastore path will be deleted
   once this migration is complete, leaving only the codepaths for Cloud SQL.
CydeWeys added 4 commits Oct 4, 2019
Copy link
Collaborator

left a comment

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


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 68 at r6 (raw file):

      saveToDatastore();
    } catch (IllegalArgumentException e) {
      logger.atInfo().withCause(e).log(

Why did you log this at info level? If it is also at severe level, you can combine this catch with the following one so that it can simplify the code.

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from d02effe to ea51dce Oct 7, 2019
Copy link
Member Author

left a comment

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


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 58 at r3 (raw file):

Previously, gbrodman wrote…

The annotations on labelsToPrices should reference the logical name of the column (revisionid) rather than the physical name of the column (revision_id). This is because the camelCase name only gets translated to the snake_case name essentially at the last step when it's written out to disk. I saw the same issue in my PR https://reviewable.io/reviews/google/nomulus/290#- at

@Index(name = "idx_registry_lock_verification_code", columnList = "verificationCode")

Done.


core/src/main/java/google/registry/schema/tld/PremiumList.java, line 46 at r4 (raw file):

Previously, gbrodman wrote…

Sorry for missing this before, but we shouldn't need the name field here -- it defaults to the entity name so that'll already be PremiumList here

(some of the test classes use this annotation because otherwise their name would be like FooTest.TestEntity instead of just TestEntity)

Done.


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 68 at r6 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Why did you log this at info level? If it is also at severe level, you can combine this catch with the following one so that it can simplify the code.

This is pre-existing code.

It is likely set up this way because we don't want to log at severe for usage errors, i.e. when someone enters a nomulus command correctly.

Copy link
Member Author

left a comment

PTAL, now with test/schema changes (and passing tests!).

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

@CydeWeys CydeWeys force-pushed the CydeWeys:persist-premiumlist-cloud-sql branch from c1fc824 to 5c7c120 Oct 7, 2019
Copy link
Collaborator

left a comment

Reviewed 1 of 2 files at r3, 2 of 3 files at r6, 5 of 7 files at r7, 5 of 5 files at r8.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 78 at r8 (raw file):

                    PremiumList.create("testlist", CurrencyUnit.USD, TEST_PRICES)));
    assertThat(thrown)
        .hasCauseThat()

remove .hasCauseThat() ?

@hstonec
hstonec approved these changes Oct 7, 2019
Copy link
Collaborator

left a comment

:lgtm:

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @CydeWeys and @gbrodman)

Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @gbrodman and @hstonec)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 78 at r8 (raw file):

Previously, hstonec (Shicong Huang) wrote…

remove .hasCauseThat() ?

Nope, the hasCauseThat() is necessary because the transaction throws a PersistenceException that wraps an IllegalArgumentException which is really what I'm trying to test.

Maybe this behavior of the transaction manager is bad, and instead of wrapping RuntimeExceptions, we should throw them directly? I believe that's how the existing ofy().transact() method works, and it is easier to work with those errors.

Copy link
Collaborator

left a comment

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 78 at r8 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Nope, the hasCauseThat() is necessary because the transaction throws a PersistenceException that wraps an IllegalArgumentException which is really what I'm trying to test.

Maybe this behavior of the transaction manager is bad, and instead of wrapping RuntimeExceptions, we should throw them directly? I believe that's how the existing ofy().transact() method works, and it is easier to work with those errors.

I see, I think the wrapping is still useful because it indicates that the operation is in a transaction and also more importantly it tells if the rollback succeeds or not.

Copy link
Collaborator

left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

Copy link
Member Author

left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hstonec)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 78 at r8 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I see, I think the wrapping is still useful because it indicates that the operation is in a transaction and also more importantly it tells if the rollback succeeds or not.

We can still keep wrapping it in the case of the rollback failing, but I don't see that it's necessary for the success case. Anyway, not relevant for this PR, but something to consider more generally.

Copy link
Member Author

left a comment

Dismissed @hstonec from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion

@CydeWeys CydeWeys dismissed hstonec’s stale review Oct 8, 2019

Shicong forgot to resolve a discussion.

@CydeWeys CydeWeys merged commit bc7f354 into google:master Oct 8, 2019
4 of 7 checks passed
4 of 7 checks passed
code-review/reviewable 1 discussion left
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
@CydeWeys CydeWeys deleted the CydeWeys:persist-premiumlist-cloud-sql branch Oct 8, 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.