Add dual write for Registrar

[hstonec]

This change is 

[CydeWeys]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 187 at r1 (raw file):

  @PostLoad
  void postLoad() {
    ImmutableList.Builder<String> builder = new ImmutableList.Builder<>();

There's definitely a better way to do all of this using Streams and .filter(Objects::nonNull)

---

core/src/main/java/google/registry/tools/UpdateRegistrarCommand.java, line 61 at r1 (raw file):

        .transact(
            () -> {
              checkArgument(

Shouldn't this logic be in the DAO?

[hstonec]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 187 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

There's definitely a better way to do all of this using Streams and .filter(Objects::nonNull)

hmm, the reasons is if any previous street line is null, we should ignore the subsequent lines because it doesn't make sense to put streetLine3 as the second element in the list when streetLine2 is null.

---

core/src/main/java/google/registry/tools/UpdateRegistrarCommand.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Shouldn't this logic be in the DAO?

This logic should be in the service layer but we don't have it.

[CydeWeys]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

  List<String> street;

  @Ignore @XmlTransient @IgnoredInDiffableMap String streetLine1;

What's the long term plan here for representing this data correctly in XML?

Also won't ignoring the differences lead to issues in verifying that things are actually the same once we try to dual read?

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 167 at r1 (raw file):

  /**
   * This callback method is used by Objectify to set streetLine[1,2,3] fields as they are not

Are you sure this works? There's no @OnLoad annotation.

What is calling this exactly? It's not a callback method per se, it's an onload trigger, right? And the Javadoc isn't formatted correctly. See https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 180 at r1 (raw file):

  /**
   * This callback method is used by Hibernate to set {@link #street} field as it is not persisted

Same Javadoc issues; see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 187 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

hmm, the reasons is if any previous street line is null, we should ignore the subsequent lines because it doesn't make sense to put streetLine3 as the second element in the list when streetLine2 is null.

That seems like an outright error, not something that should be ignored. Throw an exception then? Why would we ever have the first line null and then the second line populated?

---

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java, line 468 at r1 (raw file):

  @Override
  protected String execute() throws Exception {
    // Save registrar to datastore and output its response

Nit: Datastore

---

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java, line 474 at r1 (raw file):

    try {
      int count = 0;
      for (final List<EntityChange> batch : getCollatedEntityChangeBatches()) {

With Cloud SQL you should just be able to save them all at once in a single transaction. The whole collated entity change batches thing is an artifact of Datastore, namely, if you happen to be saving entities in different entity groups (which Registars aren't anyway), then you can only enlist 25 entity groups in a single transaction. There's no such thing as an entity group in Cloud SQL and the overall max transaction size is so larger that it's hard to imagine us ever hitting it doing anything. We shouldn't need separate batching in Cloud SQL for anything; it can always just be one big tx.

---

core/src/main/java/google/registry/tools/MutatingCommand.java, line 52 at r1 (raw file):

   * when applying a mutation that was created outside the same transaction.
   */
  protected static class EntityChange {

You should be able to revert these changes as well.

---

core/src/main/java/google/registry/tools/UpdateRegistrarCommand.java, line 61 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

This logic should be in the service layer but we don't have it.

So add it there? Don't we typically have a saveNew() method that calls persist() and an update() method that verifies existence and calls merge()?

[hstonec]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 187 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

That seems like an outright error, not something that should be ignored. Throw an exception then? Why would we ever have the first line null and then the second line populated?

ok, just realized that builder.setStreet only accepts ImmutableList<String> which prevents the situation from happening because ImmutableList doesn't allow null element.

[hstonec]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What's the long term plan here for representing this data correctly in XML?

Also won't ignoring the differences lead to issues in verifying that things are actually the same once we try to dual read?

What's the long term plan here for representing this data correctly in XML?

• I think we can just leave it as is for long term. Here are a few reasons:
  1. Our EPP response XML looks something like below, List<String> can be automatically converted while we may need to figure out how to achieve that if we use individual string:
     contact:street124 Example Dr.</contact:street>
     contact:streetSuite 200</contact:street>
  2. No need to change the existing code. Using List<String> is more convenient than using the individual fields, and we have been doing so in our code base. I deleted the getters for streetLine[1,2,3,] so streetLine[1,2,3] will become the internal state of the address and List<String> street will still be the only public facing field.

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 167 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Are you sure this works? There's no @OnLoad annotation.

What is calling this exactly? It's not a callback method per se, it's an onload trigger, right? And the Javadoc isn't formatted correctly. See https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

Yes, it works, see https://github.com/objectify/objectify/wiki/AnnotationReference . @OnLoad only works for @Entitybut not for @Embedable class like Address.

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 180 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Same Javadoc issues; see https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

Done.

---

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java, line 474 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

With Cloud SQL you should just be able to save them all at once in a single transaction. The whole collated entity change batches thing is an artifact of Datastore, namely, if you happen to be saving entities in different entity groups (which Registars aren't anyway), then you can only enlist 25 entity groups in a single transaction. There's no such thing as an entity group in Cloud SQL and the overall max transaction size is so larger that it's hard to imagine us ever hitting it doing anything. We shouldn't need separate batching in Cloud SQL for anything; it can always just be one big tx.

Done.

---

core/src/main/java/google/registry/tools/MutatingCommand.java, line 52 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

You should be able to revert these changes as well.

Done. Though, I added a method to get the collection of changed entities.

---

core/src/main/java/google/registry/tools/UpdateRegistrarCommand.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

So add it there? Don't we typically have a saveNew() method that calls persist() and an update() method that verifies existence and calls merge()?

ok, done.

[hstonec]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

What's the long term plan here for representing this data correctly in XML?

• I think we can just leave it as is for long term. Here are a few reasons:
  1. Our EPP response XML looks something like below, List<String> can be automatically converted while we may need to figure out how to achieve that if we use individual string:
     contact:street124 Example Dr.</contact:street>
     contact:streetSuite 200</contact:street>
  2. No need to change the existing code. Using List<String> is more convenient than using the individual fields, and we have been doing so in our code base. I deleted the getters for streetLine[1,2,3,] so streetLine[1,2,3] will become the internal state of the address and List<String> street will still be the only public facing field.

Also won't ignoring the differences lead to issues in verifying that things are actually the same once we try to dual read?

• On the write side, the only exposed interface for the caller to set the street is builder.setStreet, and it copies each element to streetList[1,2,3]. On the read side, we have onLoad() and postLoad() to copy the value between List<String> street and streetLine[1,2,3].

[CydeWeys]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @hstonec)

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Also won't ignoring the differences lead to issues in verifying that things are actually the same once we try to dual read?

• On the write side, the only exposed interface for the caller to set the street is builder.setStreet, and it copies each element to streetList[1,2,3]. On the read side, we have onLoad() and postLoad() to copy the value between List<String> street and streetLine[1,2,3].

The long-term plan shouldn't be to have two duplicate representations of the same data. We need to figure out a way so that the data can live in a single place, and be persisted to Cloud SQL correctly and be represented in XML accurately.

---

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java, line 476 at r2 (raw file):

          .transact(
              () ->
                  getChangedEntities().forEach(newEntity -> saveToCloudSql((Registrar) newEntity)));

And we're sure that even though you're calling saveToCloudSql() a lot of separate times, they all end up efficiently saved in a single transaction to the server?

[hstonec]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The long-term plan shouldn't be to have two duplicate representations of the same data. We need to figure out a way so that the data can live in a single place, and be persisted to Cloud SQL correctly and be represented in XML accurately.

ok, added a TODO here.

---

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java, line 476 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

And we're sure that even though you're calling saveToCloudSql() a lot of separate times, they all end up efficiently saved in a single transaction to the server?

Yeah, this is guaranteed by JpaTransactionManagerImpl. I guess you are worried about the nested jpaTm().transact(..) calls, basically, the TransactionInfo is stored in a ThreadLocal object, so every time an inner jpaTm().transact(...) will check if itself is already in the transaction, if so, it just execute the function without starting a new transaction.

[CydeWeys]

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @hstonec)

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

ok, added a TODO here.

So what is the long-term plan exactly? Depending on what that looks like, this implementation now can differ.

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 178 at r3 (raw file):

   * persisted in the Datastore. TODO(shicong): Delete this method after database migration.
   */
  void onLoad(@AlsoLoad("street") List<String> street) {

I don't see any tests of this behavior? Same for postLoad() below?

[hstonec]

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

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

So what is the long-term plan exactly? Depending on what that looks like, this implementation now can differ.

We should keep streetLine[1,2,3] and delete street then figure out how to keep the XML the same as before, i.e. map streetLine[1,2,3] to three repeated <contact:street> tags and make sure the order is kept.

[hstonec]

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/model/eppcommon/Address.java, line 178 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I don't see any tests of this behavior? Same for postLoad() below?

Done.

[hstonec]

PTAL.

Reviewable status: 0 of 14 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

[CydeWeys]

Reviewable status: 0 of 14 files reviewed, all discussions resolved