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

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 27 at r1 (raw file):

/**
 * JPA converter to for storing/retrieving {@link org.joda.time.Duration} objects. Note: this only
 * converts the values for days, hours, minutes, and seconds

The Note: can be in a separate <p>.

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 30 at r1 (raw file):

 */
@Converter(autoApply = true)
public class DurationConverter implements AttributeConverter<Duration, Object> {

Why Object instead of PGInterval?

Reviewable status: 0 of 15 files reviewed, 6 unresolved discussions (waiting on @hstonec, @mindhog, and @sarahcaseybot)

core/src/main/java/google/registry/persistence/converter/IntervalDescriptor.java, line 41 at r1 (raw file):

public static final int COLUMN_TYPE = 1426407511;

I'm just curious how this number was selected？

db/src/main/resources/sql/flyway/V20__add_relock_duration.sql, line 15 at r1 (raw file):

ALTER TABLE "RegistryLock" ADD COLUMN relock_duration interval;

we should create a new flyway file instead of modifying the existing ones.

db/src/main/resources/sql/schema/db-schema.sql.generated, line 386 at r1 (raw file):

relock_duration interval,

do we have data in this table? if so, we may want to follow our procedure to change the column.

db/src/main/resources/sql/schema/nomulus.golden.sql, line 593 at r1 (raw file):

relock_duration interval

do we have data in this table? if so, we may want to follow our procedure to change the column.

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @sarahcaseybot)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 36 at r1 (raw file):

  public Object convertToDatabaseColumn(@Nullable Duration duration) {
    if (duration == null) {
      return new PGInterval();

Don't we typically return null in this case? Does that not work for interval columns?

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 43 at r1 (raw file):

    interval.setHours(period.getHours());
    interval.setMinutes(period.getMinutes());
    interval.setSeconds(period.getSeconds());

I suspect you're going to lose millisecond precision, here. It looks like joda Duration has seconds and milliseconds as integers whereas PGInterval has double-precision seconds. if so, you'll want to convert seconds + milliseconds / 1000 to a double.

core/src/main/java/google/registry/schema/domain/RegistryLock.java, line 217 at r1 (raw file):

  }

  /** Returns true iff the lock was requested >= 1 hour ago and has not been verified. */

This seems orthogonal to this PR?

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java, line 55 at r1 (raw file):

    TestEntity persisted =
        jpaTm().transact(() -> jpaTm().getEntityManager().find(TestEntity.class, "id"));
    assertThat(persisted.duration.getMillis()).isEqualTo(Duration.standardDays(6).getMillis());

Per my earlier statement, you should do this with a duration of 6 days + some odd number of millis

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @sarahcaseybot)

db/src/main/resources/sql/schema/nomulus.golden.sql, line 593 at r1 (raw file):

@weiminyu

When we need to change the type of the relock_duration column, do we have to follow a procedure like adding a new column with the new type, change the code to use the new column, then delete the old column?

Given that we haven't launched registry lock yet, is there a simpler way to change the schema?

Reviewable status: 13 of 14 files reviewed, 10 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 28 at r3 (raw file):

 * JPA converter to for storing/retrieving {@link org.joda.time.Duration} objects.
 *
 * <p>Note: this only converts the values for days, hours, minutes, and seconds

Is this comment still accurate? Why are we throwing away years and milliseconds? We use both of those in our codebase.

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 41 at r3 (raw file):

    PGInterval interval = new PGInterval();
    Period period = new Period(duration);
    interval.setDays(period.getDays());

Why no setYears? It seems like we're just setting ourselves up for bugs down the line by not making the converter handle it, when handling it seems so simple.

Reviewable status: 10 of 14 files reviewed, 11 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 41 at r3 (raw file):

I'm not seeing a test that all of this works? I see a test that does 6 days plus 7 millis, but what about years, months, hours, minutes, and seconds?

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java, line 50 at r4 (raw file):

  @Test
  public void testRoundTrip() {

Can you add a test that

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java, line 61 at r4 (raw file):

  @Entity(name = "TestEntity") // Override entity name to avoid the nested class reference.
  @EntityForTesting
  public static class TestEntity extends ImmutableObject {

You should probably rename this to something like DurationTestEntity to prevent potential entity name conflicts causing flaky tests (which has happened a lot in the past).

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @CydeWeys, @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 41 at r3 (raw file):

Are you still working on adding this? I don't see it in here.

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 25 at r5 (raw file):

import org.postgresql.util.PGInterval;

/** JPA converter to for storing/retrieving {@link org.joda.time.Duration} objects. * */

The Javadoc needs more explanation as to how exactly all the different parts of a Joda Time Duration is stored, seeing as how there appears to be a mismatch between that and how PGInterval works.

Reviewable status: 9 of 14 files reviewed, 7 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 31 at r6 (raw file):

 * can be converted into a PGInterval, but only for the fields that have a standard number of
 * milliseconds. Therefore, there is no way to populate the months or years field of a PGInterval
 * and be confident that it is representing the exact number of milliseconds it was intended to

There's one final part missing here in the explanation, which is what happens when the Duration does include months or years. Are they ignored? Just stored as days using some flat conversion factor? Is an exception thrown? (Maybe an exception should be thrown rather than silently ignoring them?)

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java, line 32 at r6 (raw file):

/** Unit tests for {@link DurationConverter}. */
public class DurationConverterTest {

Still needs tests for what happens when the Duration includes months and/or years.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 31 at r6 (raw file):

Per out of band chat, java.util.Duration doesn't have any larger part than a day, so ignore that, but we should add a test for a large number of days (more than a year) just to rule out the possibility that anything weird is happening there, and also, Duration does support down to nanoseconds, so we should get that working and add a test of that.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/main/java/google/registry/persistence/converter/DurationConverter.java, line 31 at r6 (raw file):

Regarding nanoseconds, never mind. I was looking at the wrong Duration. We use JodaTime, which only supports granularity down to millis. So we don't need to handle anything finer than that. Also, TIL that Java's built-in Duration has 6 orders of magnitude better resolution than JodaTime.

Reviewable status: 7 of 14 files reviewed, 7 unresolved discussions (waiting on @hstonec, @mindhog, @sarahcaseybot, and @weiminyu)

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java, line 69 at r7 (raw file):

    DurationTestEntity persisted =
        jpaTm().transact(() -> jpaTm().getEntityManager().find(DurationTestEntity.class, "id"));
    assertThat(persisted.duration.getMillis()).isEqualTo(testDuration.getMillis());

Hrmmm ... I just realized .... you could put in a Duration containing hours, minutes, seconds, etc., but when you get it back out, everything has been converted to millis.

So we need tests that explicitly verify that the days, hours, minutes, seconds, and millis that you put in are exactly what you get back out.