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 RegistryLock SQL schema #243

Merged
merged 20 commits into from Sep 10, 2019

Conversation

@gbrodman
Copy link
Collaborator

commented Aug 26, 2019

This change is Reviewable

@gbrodman gbrodman added the WIP label Aug 26, 2019

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

@gbrodman gbrodman force-pushed the gbrodman:registryLockSqlObject branch from 6db9e85 to de0c380 Aug 27, 2019

@gbrodman gbrodman force-pushed the gbrodman:registryLockSqlObject branch from de0c380 to 7ba15e7 Aug 27, 2019

@gbrodman gbrodman requested a review from hstonec Aug 27, 2019

@hstonec
Copy link
Collaborator

left a comment

Looks

Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @gbrodman and @hstonec)


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

// limitations under the License.

package google.registry.schema.lock;

How about just using google.registry.schema.registrylock ? lock seems very generic to me.


util/src/main/java/google/registry/util/DateTimeUtils.java, line 105 at r1 (raw file):

   * object.
   */
  public static DateTime toDateTime(ZonedDateTime zonedDateTime) {

I also have these two functions in my pending PR :)

How about name this function totoJodaDateTime? I feel that is more clear.

@hstonec
Copy link
Collaborator

left a comment

Overall looks good to me, are you still working on this? If not may add Ben and Weimin to take a look as well.

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

@gbrodman
Copy link
Collaborator Author

left a comment

I'll add them

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


util/src/main/java/google/registry/util/DateTimeUtils.java, line 105 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I also have these two functions in my pending PR :)

How about name this function totoJodaDateTime? I feel that is more clear.

Hah! And sure.


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

Previously, hstonec (Shicong Huang) wrote…

How about just using google.registry.schema.registrylock ? lock seems very generic to me.

Done.

@gbrodman gbrodman removed the WIP label Aug 29, 2019

@gbrodman gbrodman requested review from CydeWeys and weiminyu Aug 29, 2019

@CydeWeys
Copy link
Member

left a comment

:lgtm:

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)


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

Previously, gbrodman wrote…

Done.

I don't see why we need an entire namespace just for this one thing. How about using the existing domain package?

@gbrodman
Copy link
Collaborator Author

left a comment

Quoted 33 lines of code…
        cla: yes
      
        WIP
      Shicong Huang
  22 minutes ago
assignedreview requestedgbrodman
  just now
assignedreview requested
  

  
    
       Start a new discussion
      
    
    
  



  
  
  
    
      
      
        
  single

  auto
150…</details>

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @hstonec and @weiminyu)


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

Previously, CydeWeys (Ben McIlwain) wrote…

I don't see why we need an entire namespace just for this one thing. How about using the existing domain package?

Done.

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @gbrodman, @hstonec, and @weiminyu)


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

  @Enumerated(EnumType.STRING)
  @Column(name = "lock_action", nullable = false)
  private LockAction lockAction;

Some of these fields could use Javadocs explaining their intended usage (and lifecycle). E.g. lock and unlock timestamp both start off null, then lock timestamp is set (and never unset) once the initial lock is confirmed, and then unlock timestamp is also set (and never unset) if there is ever an unlock action that is confirmed. And then, after that point, the lock is entirely immutable and historical, and if the domain is re-locked, that adds a new RegistryLock entity with a new revisionId.

Or were you thinking this would work differently, with a new id on each operation? And is that before/after verification?

Also, why are LockAction and LockStatus separate? What is the intended usage of this?

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)


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

Previously, CydeWeys (Ben McIlwain) wrote…

Some of these fields could use Javadocs explaining their intended usage (and lifecycle). E.g. lock and unlock timestamp both start off null, then lock timestamp is set (and never unset) once the initial lock is confirmed, and then unlock timestamp is also set (and never unset) if there is ever an unlock action that is confirmed. And then, after that point, the lock is entirely immutable and historical, and if the domain is re-locked, that adds a new RegistryLock entity with a new revisionId.

Or were you thinking this would work differently, with a new id on each operation? And is that before/after verification?

Also, why are LockAction and LockStatus separate? What is the intended usage of this?

I've edited this a bit to make it more clear, but it's basically what you said in your first paragraph. The user requests a lock/unlock (LockAction) and the status goes to PENDING, with the creationTimestamp set to now. Later, they verify the action, the status goes to COMPLETED, and the completionTimestamp is set, and the object becomes de facto immutable.

Any new lock/unlock actions later would be new objects.

I've changed the relevant schema in https://docs.google.com/document/d/1TMMRBbnbZ30X3EZMJdIfrEuUEZCm8JbpgQHbriUeUKA/edit# -- PTAL if you want to.

I added some javadocs.

@CydeWeys
Copy link
Member

left a comment

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


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

Previously, gbrodman wrote…

I've edited this a bit to make it more clear, but it's basically what you said in your first paragraph. The user requests a lock/unlock (LockAction) and the status goes to PENDING, with the creationTimestamp set to now. Later, they verify the action, the status goes to COMPLETED, and the completionTimestamp is set, and the object becomes de facto immutable.

Any new lock/unlock actions later would be new objects.

I've changed the relevant schema in https://docs.google.com/document/d/1TMMRBbnbZ30X3EZMJdIfrEuUEZCm8JbpgQHbriUeUKA/edit# -- PTAL if you want to.

I added some javadocs.

OK, so if LockAction is immutable, I don't understand how it would ever take the value of UNLOCK.

It would be set to LOCK when the domain is first locked ... and then that's it? It can't ever change to UNLOCK, right?

I don't understand why this field exists. What is the total lifecycle of this object, and what state does each field have at each step of the way?

Also, you have a creation timestamp and a completion timestamp, but what about when the domain is unlocked?

In my conception of the schema, a domain that is unlocked either has no row in this table whatsoever, or it only has rows that were unlocked. A domain that is locked has a row in this table that is not yet unlocked, i.e. it's halfway through the lifecycle of Nothing -> Lock Requested -> Lock Confirmed -> Unlock Requested -> Unlock Confirmed (Terminal state).

It seems to me like you're treating the locks and unlocks as separate rows? Why?

@CydeWeys
Copy link
Member

left a comment

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


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

Previously, CydeWeys (Ben McIlwain) wrote…

OK, so if LockAction is immutable, I don't understand how it would ever take the value of UNLOCK.

It would be set to LOCK when the domain is first locked ... and then that's it? It can't ever change to UNLOCK, right?

I don't understand why this field exists. What is the total lifecycle of this object, and what state does each field have at each step of the way?

Also, you have a creation timestamp and a completion timestamp, but what about when the domain is unlocked?

In my conception of the schema, a domain that is unlocked either has no row in this table whatsoever, or it only has rows that were unlocked. A domain that is locked has a row in this table that is not yet unlocked, i.e. it's halfway through the lifecycle of Nothing -> Lock Requested -> Lock Confirmed -> Unlock Requested -> Unlock Confirmed (Terminal state).

It seems to me like you're treating the locks and unlocks as separate rows? Why?

To close the loop on the in-person discussion:

I had been considering a table of registry locks, with each row showing the entire lifecycle of a given lock.

Gus has been considering a table of registry lock actions (or history entries), with each row showing just the lifecycle of a single lock or unlock action.

The advantage of the former is that it's easier to see the status of a domain, whereas the advantage of the latter is that the schema is simpler because you don't need two columns per row for things like the requested/effective time for both lock/unlock actions.

Since Gus is implementing the feature, we're going with his schema.

And some decisions we made regarding his schema:

Each domain can only have a single pending action at a time. When a new pending action is requested, it either overwrites or deletes the existing pending action, if there is one -- we haven't finalized this decision yet.

Each row will have two datetime columns, the requested datetime and the effective (or completed) datetime. Locks with a null completed datetime are pending.

In order to determine a domain's status, you query this table for rows with the given domain and a non-null completed datetime, and sort by revision ID. If there are no rows, then the domain is unlocked. If there are rows, then the action on the highest revision ID is the current state of the domain (since pending actions were already filtered out by excluding rows with null completed datetimes).

It is thus expected that, for a domain that has a lock unlock history, there will be an alternating pattern of LOCK, UNLOCK, LOCK, UNLOCK, etc., rows, with the requested datetime always before the completed datetime, which is before the requested datetime of the next highest row by revision id. If there is a pending action, there will only be one, and it will have a higher revision ID than all other rows for this domain and it will continue the alternating pattern of LOCK/UNLOCK.

Sound good?

@gbrodman
Copy link
Collaborator Author

left a comment

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


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

Previously, CydeWeys (Ben McIlwain) wrote…

To close the loop on the in-person discussion:

I had been considering a table of registry locks, with each row showing the entire lifecycle of a given lock.

Gus has been considering a table of registry lock actions (or history entries), with each row showing just the lifecycle of a single lock or unlock action.

The advantage of the former is that it's easier to see the status of a domain, whereas the advantage of the latter is that the schema is simpler because you don't need two columns per row for things like the requested/effective time for both lock/unlock actions.

Since Gus is implementing the feature, we're going with his schema.

And some decisions we made regarding his schema:

Each domain can only have a single pending action at a time. When a new pending action is requested, it either overwrites or deletes the existing pending action, if there is one -- we haven't finalized this decision yet.

Each row will have two datetime columns, the requested datetime and the effective (or completed) datetime. Locks with a null completed datetime are pending.

In order to determine a domain's status, you query this table for rows with the given domain and a non-null completed datetime, and sort by revision ID. If there are no rows, then the domain is unlocked. If there are rows, then the action on the highest revision ID is the current state of the domain (since pending actions were already filtered out by excluding rows with null completed datetimes).

It is thus expected that, for a domain that has a lock unlock history, there will be an alternating pattern of LOCK, UNLOCK, LOCK, UNLOCK, etc., rows, with the requested datetime always before the completed datetime, which is before the requested datetime of the next highest row by revision id. If there is a pending action, there will only be one, and it will have a higher revision ID than all other rows for this domain and it will continue the alternating pattern of LOCK/UNLOCK.

Sound good?

Sounds great to me, thanks for writing it up. The one change I would make would be to clarify that we no longer need a LockStatus field -- the existence (or lack thereof) of the completion time field is good enough to satisfy that requirement.

@CydeWeys
Copy link
Member

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

  creation_timestamp TIMESTAMPTZ NOT NULL,
  completion_timestamp TIMESTAMPTZ,
  verification_code TEXT UNIQUE NOT NULL,

It's not clear to me why the uniqueness constraint here makes sense.

You could imagine reusing the same well-known verification code for easier testing on non-production environments. And given that real codes are going to be generated using SecureRandom anyway, it doesn't seem necessary to attempt to enforce it at the DB layer.


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

  @Id
  @GeneratedValue(strategy = GenerationType.IDENTITY)
  @Column(name = "revision_id", nullable = false)

Are we going to use a naming strategy at some point so we don't need to manually convert every camelCased field name into snake_cased column names?

Not part of this PR necessarily, but it would be nice to have in general.


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

  private String domainName;

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

Just make this registrar_id and registrarId. We used clientId in the Datastore schema (which isn't great) and this migration gives us a chance to change to the better registrarId. 'registrarClientId' is unneccessarily wordy.


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

  @Column(name = "registrar_contact_id", nullable = false)
  private String registrarContactId;

In the new schema, registrar contacts are being renamed to registrar POCs (because overloading of the word 'contact' was often confusing).

So this should be registrarPocId, to match the new RegistrarPoc table that replaces RegistrarContacts in Datastore.


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

   */
  @Enumerated(EnumType.STRING)
  @Column(name = "lock_action", nullable = false)

Given that the table is named RegistryLock ... perhaps just naming this field/column 'action' would be sufficient?


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

  @Column(name = "is_superuser", nullable = false)
  private boolean isSuperuser;

Needs a Javadoc explaining what this is and how it's envisioned to be used.

Are we envisioning that superusers can apply a registry lock to any domain through the registrar console? And that said locks can't be overridden except by other superusers? Would the nomulus lock_domain command be modified to use superuser registry locks instead of just adding statuses to the domain name? (That would seem to make sense.)

And then, will superuser registry locks show up in the registrar's list of registry locks, just with the unlock action greyed out?

And finally, what will the registrar ID and POC ID be for superuser-created locks?

@gbrodman
Copy link
Collaborator Author

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It's not clear to me why the uniqueness constraint here makes sense.

You could imagine reusing the same well-known verification code for easier testing on non-production environments. And given that real codes are going to be generated using SecureRandom anyway, it doesn't seem necessary to attempt to enforce it at the DB layer.

Part of the reason for enforcing it here is so that duplicate writes are ignored -- this probably wouldn't end up being a huge deal, but theoretically the creation timestamp could be incorrect in that scenario. Is that worth having this restriction here?


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

Previously, CydeWeys (Ben McIlwain) wrote…

Are we going to use a naming strategy at some point so we don't need to manually convert every camelCased field name into snake_cased column names?

Not part of this PR necessarily, but it would be nice to have in general.

We should, yes, and I believe that's possible in the current code base. Weimin has b/140113767 on him currently


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

Previously, CydeWeys (Ben McIlwain) wrote…

Just make this registrar_id and registrarId. We used clientId in the Datastore schema (which isn't great) and this migration gives us a chance to change to the better registrarId. 'registrarClientId' is unneccessarily wordy.

Done.


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

Previously, CydeWeys (Ben McIlwain) wrote…

In the new schema, registrar contacts are being renamed to registrar POCs (because overloading of the word 'contact' was often confusing).

So this should be registrarPocId, to match the new RegistrarPoc table that replaces RegistrarContacts in Datastore.

Done.


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

Previously, CydeWeys (Ben McIlwain) wrote…

Given that the table is named RegistryLock ... perhaps just naming this field/column 'action' would be sufficient?

Done.


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

Previously, CydeWeys (Ben McIlwain) wrote…

Needs a Javadoc explaining what this is and how it's envisioned to be used.

Are we envisioning that superusers can apply a registry lock to any domain through the registrar console? And that said locks can't be overridden except by other superusers? Would the nomulus lock_domain command be modified to use superuser registry locks instead of just adding statuses to the domain name? (That would seem to make sense.)

And then, will superuser registry locks show up in the registrar's list of registry locks, just with the unlock action greyed out?

And finally, what will the registrar ID and POC ID be for superuser-created locks?

Responses, respectively:

Done.

I believe this would be done through the backend, used in cases like URS locks, going off the documentation in https://docs.google.com/document/d/1TMMRBbnbZ30X3EZMJdIfrEuUEZCm8JbpgQHbriUeUKA/edit?ts=5d6972bb#

The doc says that they wouldn't show up in the UI -- I don't know if that's better than greying them out

I assume for registrar ID, we'd use the registry admin client ID as a default like we do for the (Un)LockDomainCommand. For POC ID, we probably should allow that to be null, but verify that either POC ID isn't null, or this is a superuser. I've done that change.

@gbrodman
Copy link
Collaborator Author

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 15 at r6 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

You may want to use the GenerateSqlSchemaCommand on the RegistryLock class.
The Check constraint and the unique index on repo_id+revision_id is not reflected in this schema.

Also, is TEXT meant for strings with unlimited-length? Is it necessary to use it everywhere, given that it is not a standard type anyway?

Could we merge #256 before I use it here?

I assumed that we wanted to just use TEXT everywhere as a default because we weren't particularly concerned about space.


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

Previously, weiminyu (Weimin Yu) wrote…

Yes. If all writes are through this class using hibernate, then there is no need for a constraint on the database table.

Ah, then I think we should be ok and I can remove it.

@CydeWeys
Copy link
Member

left a comment

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


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

Previously, gbrodman wrote…

Do we join to find the domain name in other locations in the code base? Here, it seems simpler to store it since we wouldn't have to depend on any Datastore entities, but it might be the correct solution to just store one or the other.

I was thinking of the ultimate final version where all data is stored in Cloud SQL. In the transitional period where the EPP resources are still in Datastore, I can see this being useful to store here as well.

Might want to leave a TODO that the long-term plan would be to remove this column and instead JOIN to the Domain table, once that's all migrated over into Cloud SQL.

@CydeWeys
Copy link
Member

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, gbrodman wrote…

I don't think the issue is if the program crashes, but rather if we have some retry logic (which may be built in? I'm not sure.) that attempts a DB-level write of the same object after a DB-level write "failed" but actually succeeded.

Then that shouldn't be a problem, as it wouldn't generate a new id; it'd use the existing one it already had. And that would fail because a row with that id would in fact already exist in the table. We don't need an additional uniqueness constraint when there's already a uniqueness constraint on the id plus the repo id.


db/src/main/resources/sql/schema/registry_lock.sql, line 15 at r6 (raw file):

Previously, gbrodman wrote…

Could we merge #256 before I use it here?

I assumed that we wanted to just use TEXT everywhere as a default because we weren't particularly concerned about space.

https://www.depesz.com/2010/03/02/charx-vs-varcharx-vs-varchar-vs-text/

It sounds like the winning move is to always use TEXT, as there are no internal representation differences in PostgreSQL anyway, and the only "advantages" of the varchar types is that you're putting the DB in control of padding all your strings to a particular length, which is probably not something we'd ever desire.

@CydeWeys
Copy link
Member

left a comment

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


util/src/main/java/google/registry/util/DateTimeUtils.java, line 105 at r1 (raw file):

Previously, gbrodman wrote…

Hah! And sure.

Can you split these out into a separate PR and submit it now, so it's not blocked on the rest of this review? I need to use these too.

@gbrodman
Copy link
Collaborator Author

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Then that shouldn't be a problem, as it wouldn't generate a new id; it'd use the existing one it already had. And that would fail because a row with that id would in fact already exist in the table. We don't need an additional uniqueness constraint when there's already a uniqueness constraint on the id plus the repo id.

In this situation, would it generate a new revisionId or would it use the old one?


db/src/main/resources/sql/schema/registry_lock.sql, line 15 at r6 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

https://www.depesz.com/2010/03/02/charx-vs-varcharx-vs-varchar-vs-text/

It sounds like the winning move is to always use TEXT, as there are no internal representation differences in PostgreSQL anyway, and the only "advantages" of the varchar types is that you're putting the DB in control of padding all your strings to a particular length, which is probably not something we'd ever desire.

agree


util/src/main/java/google/registry/util/DateTimeUtils.java, line 105 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can you split these out into a separate PR and submit it now, so it's not blocked on the rest of this review? I need to use these too.

Done. #260


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

Previously, CydeWeys (Ben McIlwain) wrote…

I was thinking of the ultimate final version where all data is stored in Cloud SQL. In the transitional period where the EPP resources are still in Datastore, I can see this being useful to store here as well.

Might want to leave a TODO that the long-term plan would be to remove this column and instead JOIN to the Domain table, once that's all migrated over into Cloud SQL.

Oh, that makes total sense then. Done.

@gbrodman
Copy link
Collaborator Author

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, gbrodman wrote…

In this situation, would it generate a new revisionId or would it use the old one?

It seems like it would often generate a new revision ID, @weimin?

@CydeWeys
Copy link
Member

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, gbrodman wrote…

It seems like it would often generate a new revision ID, @weimin?

I'm struggling to think of why it would use a different ID on every retry. That seems pathological and guaranteed to produce unintended errors in a wide variety of situations.

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @hstonec, @weimin, and @weiminyu)


db/src/main/resources/sql/schema/registry_lock.sql, line 24 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm struggling to think of why it would use a different ID on every retry. That seems pathological and guaranteed to produce unintended errors in a wide variety of situations.

OK then. Because the retry logic is handled at the application level, and because this is something done by the user, I don't think we need to retry -- we can just give an error on the confirmation page if appropriate, and the user can try again. That means we don't need to worry about verification code uniqueness

@gbrodman

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2019

PTAL, should be good to go here thanks

@gbrodman gbrodman requested review from CydeWeys, weiminyu and hstonec Sep 10, 2019

@CydeWeys
Copy link
Member

left a comment

:lgtm:

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)

@weiminyu
Copy link
Collaborator

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 15 at r6 (raw file):

Previously, gbrodman wrote…

agree

I think this is still missing the unique index 'idx_registry_lock_repo_id_revision_id'

@weiminyu
Copy link
Collaborator

left a comment

Reviewed 1 of 5 files at r8.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @hstonec)

@gbrodman
Copy link
Collaborator Author

left a comment

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


db/src/main/resources/sql/schema/registry_lock.sql, line 15 at r6 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

I think this is still missing the unique index 'idx_registry_lock_repo_id_revision_id'

Done.

@gbrodman gbrodman force-pushed the gbrodman:registryLockSqlObject branch from 803ef56 to 1505af3 Sep 10, 2019

@weiminyu
Copy link
Collaborator

left a comment

Reviewed 1 of 5 files at r8, 2 of 2 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hstonec)

@gbrodman gbrodman merged commit 401653a into google:master Sep 10, 2019

4 of 7 checks passed

code-review/reviewable 2 discussions left (hstonec)
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

@gbrodman gbrodman deleted the gbrodman:registryLockSqlObject branch Sep 10, 2019

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