-
Notifications
You must be signed in to change notification settings - Fork 272
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
Refactor away inner transaction in AllocationToken.loadToken() #2127
base: master
Are you sure you want to change the base?
Conversation
3d7cce1
to
e9302d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, butverifyAllocationTokenIfPresent()
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?
This makes clever use of Assertions.assertDoesNotThrow() to solve the problem of not being able to wrap code that throws checked exceptions in a lambda function (as is typically necessary when creating a new transaction). Note that this approach is suitable for test code only; real code will simply need to have real, non-anonymous methods that declare checked exceptions, and then call transact() on the named methods (our codebase already has plenty of examples of this).
e9302d6
to
f070528
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @gbrodman and @jianglai)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This change is