Refactor away inner transaction in AllocationToken.loadToken()

[CydeWeys]

This change is 

[gbrodman]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @jianglai)

---

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 186 at r1 (raw file):

        allocationTokenExtension.map(
            tokenExtension ->
                tm().transact(

it seems like we have (this is not new) a ton of mini-transactions inside DomainCheckFlow. Is it better to have a bunch of small read-only transactions (thus entering/exiting transactions more frequently) or is it better to have one larger read-only transaction (possibly keeping locks for longer, but avoiding the transaction overhead)?

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 143 at r1 (raw file):

    when(allocationTokenExtension.getAllocationToken()).thenReturn("tokeN");
    assertThat(
            tm().transact(

what about a test method that encapsulates this? Like, calling something like DatabaseHelper.transactWithoutThrowing(() -> flowUtils.verify.....))

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

        .that(
            tm().transact(
                    () ->

i think it might make more sense to move the transact call one level inward, to keep the two assert calls closer to each other

[CydeWeys]

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

---

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 186 at r1 (raw file):

Previously, gbrodman wrote…

it seems like we have (this is not new) a ton of mini-transactions inside DomainCheckFlow. Is it better to have a bunch of small read-only transactions (thus entering/exiting transactions more frequently) or is it better to have one larger read-only transaction (possibly keeping locks for longer, but avoiding the transaction overhead)?

Yes, see #2129 for a better solution to the overall problem that will prevent us from having to add lots of little transactions into flows (I wasn't liking that either). That will go in first and then I will fix up this PR subsequently.

[CydeWeys]

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

---

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 186 at r1 (raw file):

Previously, gbrodman wrote…

it seems like we have (this is not new) a ton of mini-transactions inside DomainCheckFlow. Is it better to have a bunch of small read-only transactions (thus entering/exiting transactions more frequently) or is it better to have one larger read-only transaction (possibly keeping locks for longer, but avoiding the transaction overhead)?

Yes, see #2129 for a better solution to the overall problem that will prevent us from having to add lots of little transactions into flows (I wasn't liking that either). That will go in first and then I will fix up this PR subsequently.

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 143 at r1 (raw file):

Previously, gbrodman wrote…

what about a test method that encapsulates this? Like, calling something like DatabaseHelper.transactWithoutThrowing(() -> flowUtils.verify.....))

It doesn't work because the issue is with the exceptions themselves being thrown in the lambda.

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 143 at r1 (raw file):

Previously, gbrodman wrote…

what about a test method that encapsulates this? Like, calling something like DatabaseHelper.transactWithoutThrowing(() -> flowUtils.verify.....))

It doesn't work because the issue is with the exceptions themselves being thrown in the lambda.

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, gbrodman wrote…

i think it might make more sense to move the transact call one level inward, to keep the two assert calls closer to each other

It doesn't work. assertThrows() doesn't throw a checked exception, so it's eligible to be in the lambda, but verifyAllocationTokenIfPresent() does throw a checked exception, so it can't be in the lambda without something wrapping it that swallows that exception without rethrowing it.

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, gbrodman wrote…

i think it might make more sense to move the transact call one level inward, to keep the two assert calls closer to each other

It doesn't work. assertThrows() doesn't throw a checked exception, so it's eligible to be in the lambda, but verifyAllocationTokenIfPresent() does throw a checked exception, so it can't be in the lambda without something wrapping it that swallows that exception without rethrowing it.

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, gbrodman wrote…

i think it might make more sense to move the transact call one level inward, to keep the two assert calls closer to each other

It doesn't work. assertThrows() doesn't throw a checked exception, so it's eligible to be in the lambda, but verifyAllocationTokenIfPresent() does throw a checked exception, so it can't be in the lambda without something wrapping it that swallows that exception without rethrowing it.

[gbrodman]

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

---

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 186 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Yes, see #2129 for a better solution to the overall problem that will prevent us from having to add lots of little transactions into flows (I wasn't liking that either). That will go in first and then I will fix up this PR subsequently.

got it, sgtm

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 143 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It doesn't work because the issue is with the exceptions themselves being thrown in the lambda.

ugh right idk what i was thinking

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It doesn't work. assertThrows() doesn't throw a checked exception, so it's eligible to be in the lambda, but verifyAllocationTokenIfPresent() does throw a checked exception, so it can't be in the lambda without something wrapping it that swallows that exception without rethrowing it.

hmmm yeah, what about moving the transact call outward so it's the first line in the method?

[CydeWeys]

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

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, gbrodman wrote…

hmmm yeah, what about moving the transact call outward so it's the first line in the method?

I mean it's possible, but is it better? Personally I'd rather see the asserts at the top level rather than the meaningless transactional wrapper.

[CydeWeys]

PTAL

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

[CydeWeys]

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

---

core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java line 186 at r1 (raw file):

Previously, gbrodman wrote…

got it, sgtm

Done.

[gbrodman]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jianglai)

---

core/src/test/java/google/registry/flows/domain/token/AllocationTokenFlowUtilsTest.java line 482 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I mean it's possible, but is it better? Personally I'd rather see the asserts at the top level rather than the meaningless transactional wrapper.

eh